| Message ID | 20190722154538.5314-1-david@lechnology.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | new driver for TI eQEP | expand |
On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote: > This series adds device tree bindings and a new counter driver for the Texas > Instruments Enhanced Quadrature Encoder Pulse (eQEP). > > As mentioned in one of the commit messages, to start with, the driver only > supports reading the current counter value and setting the min/max values. > Other features can be added on an as-needed basis. > > The only other feature I am interested in is adding is getting time data in > order to calculate the rotational speed of a motor. However, there probably > needs to be a higher level discussion of how this can fit into the counter > subsystem in general first. I believe exposing some sort of time data has merit. Quadrature counter devices in particular are commonly used for position tracking of automation systems, and such systems would benefit from velocity/speed information. So let's try to introduce that sort of functionality in this driver if possible. First, let's discuss your specific use case and requirements, and hopefully we can generalize it enough to be of use for future drivers. From your description, it sounds like you're attaching some sort of rotary encoder to the eQEP device. Is that correct? What sort of time data are you hoping to use; does the eQEP device provide a clock value, or would you be grabbing a timestamp from the system? I'm not sure yet if it would make sense to expose rotational speed directly as an attribute. If we were to expose just the count value and timestamp since the last read, that should be enough for a user to compute the delta and derive speed. I'll think more about this since some devices may simplify that case if the hardware is able to compute the speed for us. William Breathitt Gray
On 7/25/19 7:40 AM, William Breathitt Gray wrote: > On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote: >> This series adds device tree bindings and a new counter driver for the Texas >> Instruments Enhanced Quadrature Encoder Pulse (eQEP). >> >> As mentioned in one of the commit messages, to start with, the driver only >> supports reading the current counter value and setting the min/max values. >> Other features can be added on an as-needed basis. >> >> The only other feature I am interested in is adding is getting time data in >> order to calculate the rotational speed of a motor. However, there probably >> needs to be a higher level discussion of how this can fit into the counter >> subsystem in general first. > > I believe exposing some sort of time data has merit. Quadrature counter > devices in particular are commonly used for position tracking of > automation systems, and such systems would benefit from velocity/speed > information. So let's try to introduce that sort of functionality in this > driver if possible. > > First, let's discuss your specific use case and requirements, and hopefully we > can generalize it enough to be of use for future drivers. From your description, > it sounds like you're attaching some sort of rotary encoder to the eQEP device. > Is that correct? What sort of time data are you hoping to use; does the eQEP > device provide a clock value, or would you be grabbing a timestamp from the > system? My use case is robotics using LEGO MINDSTORMS. More specifically, I am using motors that have a cheap optical rotary encoder (plastic wheel and infrared LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count is defined as the rising edge or falling edge of the A signal. We are looking at anywhere from 0 to around 2000 counts per second. We use the speed as feedback in a control algorithm to drive the motor at a constant speed. The control loop updates on the order of 1 to 10 ms. Because the encoder resolution and speeds are relatively low, we are currently logging a timestamp for each count. If no count occurs for 50ms, then we log the same count again with a new timestamp (otherwise we would never see 0 speed). To get the actual speed, we find the first timestamp > 20 ms before the current timestamp then compute the speed as the change in position divided by the change in time between these two samples. This give a fairly accurate speed across most of the range, but does get a bit noisy once we get below 100 counts per second. It also means that we need a ring buffer that holds about 50 samples. The timestamp itself comes from the eQEP, not the system. There are latching registers to ensure that the timestamp read is from exactly the moment when the count register was read. > I'm not sure yet if it would make sense to expose rotational speed directly as > an attribute. If we were to expose just the count value and timestamp since the > last read, that should be enough for a user to compute the delta and derive > speed. I'll think more about this since some devices may simplify that case if > the hardware is able to compute the speed for us. > I agree that it probably doesn't make sense to expect drivers to compute the speed. There isn't really a general way to do that works for an arbitrary speed. For example at high speeds, it is better to just look at the change in counts over a fixed interval rather than triggering a timestamp based on a certain number of counts. I also don't think having a timestamp sysfs attribute would be very useful. To make it work at all, I think it would have to be implemented such that it returns the timestamp for the count that was most recently read via sysfs. And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/ timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not a negligible amount of time. I noticed that several of the other counter drivers also register an IIO device. So this got me thinking that perhaps the counter subsystem should just be for configuring a counter device an then the IIO subsystem should be used for triggers and ring buffers. For the general case a counter device could have two possible triggers. One that triggers an interrupt after X counts and another that triggers with a period of T nanoseconds (or microseconds). Both triggers would add a count/timestamp pair to an IIO ring buffer. To fully reproduce our current methodology the first trigger would actually need two configurable settings, the count X that triggers every X counts and a watchdog time setting (using terminology from eQEP docs) that will also trigger if and only if the count does not change before the time has elapsed. Note, this is different from the other proposed time based trigger which would cause a trigger interrupt at a fixed period regardless of whether the count changed or not. --- Thinking more generally though, I think what I would propose is adding a new component to the existing list of Count, Signal and Synapse. The new component could be called Event. Event would be more general than the trigger conditions I have just discussed. In addition to those two, it could be any event generated by the hardware, such as an error condition or a change in direction. Drivers could register an arbitrary number of events for each Count, so we would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few standard attributes, like "name" and "enable". Configurable events would need ext attributes to allow configuration. However, I see that there are already preset and error_noise "events" for count objects, so maybe we don't do the eventY thing and keep it flatter (or is the counter subsystem still considered in "staging" where breaking ABI changes could be made?). When thinking about what events would actually do when enabled though, it seems like we should be using IIO events and triggers (we have found reading sysfs attributes to be insufficient performance-wise). It seems like unnecessary work to reproduce all of this in the counter subsystem. Which makes me wonder if it would be better to have counter devices just be a different device type (i.e. different struct device_type for dev->type) in the IIO subsystem instead of creating a completely new subsystem.
On Thu, 25 Jul 2019 17:52:21 -0500 David Lechner <david@lechnology.com> wrote: > On 7/25/19 7:40 AM, William Breathitt Gray wrote: > > On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote: > >> This series adds device tree bindings and a new counter driver for the Texas > >> Instruments Enhanced Quadrature Encoder Pulse (eQEP). > >> > >> As mentioned in one of the commit messages, to start with, the driver only > >> supports reading the current counter value and setting the min/max values. > >> Other features can be added on an as-needed basis. > >> > >> The only other feature I am interested in is adding is getting time data in > >> order to calculate the rotational speed of a motor. However, there probably > >> needs to be a higher level discussion of how this can fit into the counter > >> subsystem in general first. > > > > I believe exposing some sort of time data has merit. Quadrature counter > > devices in particular are commonly used for position tracking of > > automation systems, and such systems would benefit from velocity/speed > > information. So let's try to introduce that sort of functionality in this > > driver if possible. > > > > First, let's discuss your specific use case and requirements, and hopefully we > > can generalize it enough to be of use for future drivers. From your description, > > it sounds like you're attaching some sort of rotary encoder to the eQEP device. > > Is that correct? What sort of time data are you hoping to use; does the eQEP > > device provide a clock value, or would you be grabbing a timestamp from the > > system? > > My use case is robotics using LEGO MINDSTORMS. More specifically, I am using > motors that have a cheap optical rotary encoder (plastic wheel and infrared > LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count > is defined as the rising edge or falling edge of the A signal. We are looking at > anywhere from 0 to around 2000 counts per second. We use the speed as feedback in > a control algorithm to drive the motor at a constant speed. The control loop > updates on the order of 1 to 10 ms. > > Because the encoder resolution and speeds are relatively low, we are currently > logging a timestamp for each count. If no count occurs for 50ms, then we log the > same count again with a new timestamp (otherwise we would never see 0 speed). To > get the actual speed, we find the first timestamp > 20 ms before the current > timestamp then compute the speed as the change in position divided by the change > in time between these two samples. This give a fairly accurate speed across most > of the range, but does get a bit noisy once we get below 100 counts per second. > It also means that we need a ring buffer that holds about 50 samples. > > The timestamp itself comes from the eQEP, not the system. There are latching > registers to ensure that the timestamp read is from exactly the moment when > the count register was read. > > > > I'm not sure yet if it would make sense to expose rotational speed directly as > > an attribute. If we were to expose just the count value and timestamp since the > > last read, that should be enough for a user to compute the delta and derive > > speed. I'll think more about this since some devices may simplify that case if > > the hardware is able to compute the speed for us. > > > > I agree that it probably doesn't make sense to expect drivers to compute the > speed. There isn't really a general way to do that works for an arbitrary > speed. For example at high speeds, it is better to just look at the change > in counts over a fixed interval rather than triggering a timestamp based on > a certain number of counts. Worth noting perhaps that userspace has the same problem with knowing the right approach... > > I also don't think having a timestamp sysfs attribute would be very useful. > To make it work at all, I think it would have to be implemented such that > it returns the timestamp for the count that was most recently read via sysfs. > And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/ > timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not > a negligible amount of time. > > I noticed that several of the other counter drivers also register an IIO > device. So this got me thinking that perhaps the counter subsystem should > just be for configuring a counter device an then the IIO subsystem should > be used for triggers and ring buffers. > > For the general case a counter device could have two possible triggers. > One that triggers an interrupt after X counts and another that triggers > with a period of T nanoseconds (or microseconds). Both triggers would add > a count/timestamp pair to an IIO ring buffer. > > To fully reproduce our current methodology the first trigger would actually > need two configurable settings, the count X that triggers every X counts and > a watchdog time setting (using terminology from eQEP docs) that will also > trigger if and only if the count does not change before the time has elapsed. > Note, this is different from the other proposed time based trigger which > would cause a trigger interrupt at a fixed period regardless of whether > the count changed or not. > > --- > > Thinking more generally though, I think what I would propose is adding a new > component to the existing list of Count, Signal and Synapse. The new component > could be called Event. Event would be more general than the trigger conditions > I have just discussed. In addition to those two, it could be any event > generated by the hardware, such as an error condition or a change in direction. > > Drivers could register an arbitrary number of events for each Count, so we > would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few > standard attributes, like "name" and "enable". Configurable events would need > ext attributes to allow configuration. > > However, I see that there are already preset and error_noise "events" for > count objects, so maybe we don't do the eventY thing and keep it flatter (or > is the counter subsystem still considered in "staging" where breaking ABI > changes could be made?). No. I would say it can be extended but compatibility needs to be maintained. > > When thinking about what events would actually do when enabled though, it > seems like we should be using IIO events and triggers (we have found reading > sysfs attributes to be insufficient performance-wise). It seems like unnecessary > work to reproduce all of this in the counter subsystem. Which makes me wonder if > it would be better to have counter devices just be a different device type (i.e. > different struct device_type for dev->type) in the IIO subsystem instead of > creating a completely new subsystem. Hmm. That might have sort of worked, rather than the full split we went with when it became clear counters really didn't fit the IIO model, but it may not have come out cleanly and wouldn't have shared all that much code (I think...) In particular the buffered paths in IIO present a large and complex interface which would need to be replace given the different fundamental structures in the counter abstraction. It may be that the best thing to do here is to use very similar concepts and implement some sort of kfifo interface for counters. There may well even be code to share, but I don't think we want to go back to trying to handle these as devices within IIO. Tricky to know how well this would work without a prototype though... Jonathan
On Thu, Jul 25, 2019 at 05:52:21PM -0500, David Lechner wrote: > On 7/25/19 7:40 AM, William Breathitt Gray wrote: > > On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote: > >> This series adds device tree bindings and a new counter driver for the Texas > >> Instruments Enhanced Quadrature Encoder Pulse (eQEP). > >> > >> As mentioned in one of the commit messages, to start with, the driver only > >> supports reading the current counter value and setting the min/max values. > >> Other features can be added on an as-needed basis. > >> > >> The only other feature I am interested in is adding is getting time data in > >> order to calculate the rotational speed of a motor. However, there probably > >> needs to be a higher level discussion of how this can fit into the counter > >> subsystem in general first. > > > > I believe exposing some sort of time data has merit. Quadrature counter > > devices in particular are commonly used for position tracking of > > automation systems, and such systems would benefit from velocity/speed > > information. So let's try to introduce that sort of functionality in this > > driver if possible. > > > > First, let's discuss your specific use case and requirements, and hopefully we > > can generalize it enough to be of use for future drivers. From your description, > > it sounds like you're attaching some sort of rotary encoder to the eQEP device. > > Is that correct? What sort of time data are you hoping to use; does the eQEP > > device provide a clock value, or would you be grabbing a timestamp from the > > system? > > My use case is robotics using LEGO MINDSTORMS. More specifically, I am using > motors that have a cheap optical rotary encoder (plastic wheel and infrared > LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count > is defined as the rising edge or falling edge of the A signal. We are looking at > anywhere from 0 to around 2000 counts per second. We use the speed as feedback in > a control algorithm to drive the motor at a constant speed. The control loop > updates on the order of 1 to 10 ms. > > Because the encoder resolution and speeds are relatively low, we are currently > logging a timestamp for each count. If no count occurs for 50ms, then we log the > same count again with a new timestamp (otherwise we would never see 0 speed). To > get the actual speed, we find the first timestamp > 20 ms before the current > timestamp then compute the speed as the change in position divided by the change > in time between these two samples. This give a fairly accurate speed across most > of the range, but does get a bit noisy once we get below 100 counts per second. > It also means that we need a ring buffer that holds about 50 samples. > > The timestamp itself comes from the eQEP, not the system. There are latching > registers to ensure that the timestamp read is from exactly the moment when > the count register was read. So if I understand correctly, there are two registers you're reading: a count register and a timestamp register. The count register is updated by the rotation of the motor shaft, while the timestamp register is updated by reading the count register (thus logging the time associated with the read count value). > > I'm not sure yet if it would make sense to expose rotational speed directly as > > an attribute. If we were to expose just the count value and timestamp since the > > last read, that should be enough for a user to compute the delta and derive > > speed. I'll think more about this since some devices may simplify that case if > > the hardware is able to compute the speed for us. > > > > I agree that it probably doesn't make sense to expect drivers to compute the > speed. There isn't really a general way to do that works for an arbitrary > speed. For example at high speeds, it is better to just look at the change > in counts over a fixed interval rather than triggering a timestamp based on > a certain number of counts. This is a good point. Depending on the resolution the user cares about, they may be more interested in the speed over a short time interval versus a long time interval. It doesn't seem practical to have the driver try to handle all possible speed calculations when the user can decide themselves how best to use the data. > I also don't think having a timestamp sysfs attribute would be very useful. > To make it work at all, I think it would have to be implemented such that > it returns the timestamp for the count that was most recently read via sysfs. > And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/ > timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not > a negligible amount of time. This is a concern I've had as well. The sysfs interface is useful in that it provides an intuitive and human-friendly way to expose data about devices. But as you note, there is considerable overhead in the amount of syscalls we have to make to interact with multiple attributes. One solution that may work is providing a character device interface in addition to the sysfs interface. I believe that should reduce the syscall overhead since a user can pass in a data structure with a configuration defining what data/actions they want, and receive back all data in a single syscall. I think concern over latency was one of the reasons the GPIO subsystem gained a character device interface as well. It's an addition to the Counter subsystem that is worth considering, but the possible downsides to such an interface should also be investigated. > I noticed that several of the other counter drivers also register an IIO > device. So this got me thinking that perhaps the counter subsystem should > just be for configuring a counter device an then the IIO subsystem should > be used for triggers and ring buffers. > > For the general case a counter device could have two possible triggers. > One that triggers an interrupt after X counts and another that triggers > with a period of T nanoseconds (or microseconds). Both triggers would add > a count/timestamp pair to an IIO ring buffer. > > To fully reproduce our current methodology the first trigger would actually > need two configurable settings, the count X that triggers every X counts and > a watchdog time setting (using terminology from eQEP docs) that will also > trigger if and only if the count does not change before the time has elapsed. > Note, this is different from the other proposed time based trigger which > would cause a trigger interrupt at a fixed period regardless of whether > the count changed or not. The counter drivers in the kernel right now are registering IIO devices in order to keep the preexisting (but now deprecated) IIO Counter interface working for these devices -- some users may be using this older interface so we don't want to remove it cold turkey. Regardless, there's nothing the prevents incorporating the IIO interface with your Counter drivers; in fact, in some circumstances it's better that you do just that. The key idea to recognize is how the Counter subsystem differs from the IIO subsystem on a conceptual level: the IIO subsystem provides an interface for your device by describing it on a hardware level, whereas the Counter subsystem provides an interface for your device by describing it on a more abstract level. What I mean is that every interface interaction in the Counter subsystem relates to the abstract concept of an ideal "counter device" (Counts, Synapses, Signals); if a device functionality or data does not relate directly to those ideal counter device components, then the Counter subsystem isn't that right interface for it. For example, it makes sense to have an "enable" attribute or "present" attribute, because these functionalities/data are directly related to the Count, Synapse, and Signal components conceptually. However, in the Counter subsystem you will likely not see something like the IIO "in_voltageY_supply_raw" attribute -- not because that data is not useful to know about for the operation of the counter device hardware, but because it is outside the scope of the Counter subsystem paradigm (i.e. it does not directly related to Counts, Synapses, or Signals). As such, this would be a case where the counter driver should register both a Counter device and IIO device, one to handle the counter device on an abstract level while the other provides an interface for control of the more specific hardware details. > --- > > Thinking more generally though, I think what I would propose is adding a new > component to the existing list of Count, Signal and Synapse. The new component > could be called Event. Event would be more general than the trigger conditions > I have just discussed. In addition to those two, it could be any event > generated by the hardware, such as an error condition or a change in direction. > > Drivers could register an arbitrary number of events for each Count, so we > would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few > standard attributes, like "name" and "enable". Configurable events would need > ext attributes to allow configuration. > > However, I see that there are already preset and error_noise "events" for > count objects, so maybe we don't do the eventY thing and keep it flatter (or > is the counter subsystem still considered in "staging" where breaking ABI > changes could be made?). The components for handling events already exist in the Counter interface paradigm: Signals and Synapses. Although, the Counter subsystem is currently lacking the implementation (I still need to code in support for interrupts and such), the paradigm itself supports the concept of events and triggers. Recall that the Counter subsystem represents hardware via the abstraction of an idealized "counter device". This is important to understand because it means that Signals are not necessarily limited to the physical wires of the hardware. To summarize the Counter interface paradigm: * A Signal is a stream of data to be evaluated. * A Synapse is a trigger condition based on the evaluation of the data streams (i.e. the Signals). * A Count is the accumulation of the effects of Synapses (i.e. the triggers). As such, in order to represent an event, you would add in a Signal to represent the stream of device events, and a Synapse defining the specific event that will trigger the action. I'll give an example in order to demonstrate what I mean. A simple clock can be conceptualize as a proper counter device: an oscillation is a Signal, a rising edge from that oscillation line can be the Synapse, and the current clock value is the Count. Count Synapse Signal ----- ------- ------ +---------------------+ | Data: Clock ticks | Rising Edge _____________ | Function: Increase | <------------- / Oscillation \ | | _________________ +---------------------+ Now, in order to represent your timestamping clock we need two Signals: a simple clock and an event stream. The simple clock is the source of the current clock ticks we will store, while the event stream provides the rotation count register read notification that will trigger the timestamp. Count Synapse Signal ----- ------- ------ +-------------------------------+ | Data: Timestamp | None _______ | Function: Current clock ticks | <------------ / Clock \ | | ___________ | | | | Read event ________ | | <------------ / Events \ | | ____________ +-------------------------------+ Note that in this case both Signals either do not exist in or are not exposed by the hardware (maybe the simple clock is exposed, but it's not necessary to be) -- they are meant to be abstract representations of the components of the timestamp clock as an idealized "counter device". By organizing the timestamp clock in this way, we can control and configure the components using the standard Counter interface: common attributes such as "name", "preset", "enable", etc. can now be exposed to users like every other counter device component. In theory we can sleep on the timestamp count attribute read (or character device equivalent if we go down that route), and be woken when an event triggers updating the timestamp value. However, the current Counter subsystem implementation is lacking the code for this so it needs to be added to the core functionality first. > When thinking about what events would actually do when enabled though, it > seems like we should be using IIO events and triggers (we have found reading > sysfs attributes to be insufficient performance-wise). It seems like unnecessary > work to reproduce all of this in the counter subsystem. Which makes me wonder if > it would be better to have counter devices just be a different device type (i.e. > different struct device_type for dev->type) in the IIO subsystem instead of > creating a completely new subsystem. I plan on adding interrupt support for the 104-QUAD-8 counter driver since this device has some useful interrupts on configured threshold conditions and such, so having the ability to handle an event rather than constantly read and loop is something I want to have in the Counter subsystem. It's possible that I can reuse some code from the IIO subsystem, as Jonathan pointed out, but overall I believe these should be separate subsystems. From the reasons described above, the IIO subsystem and Counter subsystem have different goals and thus different implementations. I don't think that's a bad thing, and we can share code in the few cases where the two may overlap. Regarding whether to use IIO events and triggers within the TI eQEP counter driver, I think we should wait for a proper Counter subsystem implementation to be added first. My fear is that we'll have a similar situation as what happened with IIO_COUNT, where we'll have to keep a IIO interface present with a newer Counter interface. If adding in event support to the Counter subsystem will take too long, we can add this TI eQEP driver as-is now and later add in the timestamp support. William Breathitt Gray
On 7/29/19 11:45 PM, William Breathitt Gray wrote: > On Thu, Jul 25, 2019 at 05:52:21PM -0500, David Lechner wrote: >> On 7/25/19 7:40 AM, William Breathitt Gray wrote: >>> On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote: >>>> This series adds device tree bindings and a new counter driver for the Texas >>>> Instruments Enhanced Quadrature Encoder Pulse (eQEP). >>>> >>>> As mentioned in one of the commit messages, to start with, the driver only >>>> supports reading the current counter value and setting the min/max values. >>>> Other features can be added on an as-needed basis. >>>> >>>> The only other feature I am interested in is adding is getting time data in >>>> order to calculate the rotational speed of a motor. However, there probably >>>> needs to be a higher level discussion of how this can fit into the counter >>>> subsystem in general first. >>> >>> I believe exposing some sort of time data has merit. Quadrature counter >>> devices in particular are commonly used for position tracking of >>> automation systems, and such systems would benefit from velocity/speed >>> information. So let's try to introduce that sort of functionality in this >>> driver if possible. >>> >>> First, let's discuss your specific use case and requirements, and hopefully we >>> can generalize it enough to be of use for future drivers. From your description, >>> it sounds like you're attaching some sort of rotary encoder to the eQEP device. >>> Is that correct? What sort of time data are you hoping to use; does the eQEP >>> device provide a clock value, or would you be grabbing a timestamp from the >>> system? >> >> My use case is robotics using LEGO MINDSTORMS. More specifically, I am using >> motors that have a cheap optical rotary encoder (plastic wheel and infrared >> LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count >> is defined as the rising edge or falling edge of the A signal. We are looking at >> anywhere from 0 to around 2000 counts per second. We use the speed as feedback in >> a control algorithm to drive the motor at a constant speed. The control loop >> updates on the order of 1 to 10 ms. >> >> Because the encoder resolution and speeds are relatively low, we are currently >> logging a timestamp for each count. If no count occurs for 50ms, then we log the >> same count again with a new timestamp (otherwise we would never see 0 speed). To >> get the actual speed, we find the first timestamp > 20 ms before the current >> timestamp then compute the speed as the change in position divided by the change >> in time between these two samples. This give a fairly accurate speed across most >> of the range, but does get a bit noisy once we get below 100 counts per second. >> It also means that we need a ring buffer that holds about 50 samples. >> >> The timestamp itself comes from the eQEP, not the system. There are latching >> registers to ensure that the timestamp read is from exactly the moment when >> the count register was read. > > So if I understand correctly, there are two registers you're reading: a > count register and a timestamp register. The count register is updated > by the rotation of the motor shaft, while the timestamp register is > updated by reading the count register (thus logging the time associated > with the read count value). That is correct. > >>> I'm not sure yet if it would make sense to expose rotational speed directly as >>> an attribute. If we were to expose just the count value and timestamp since the >>> last read, that should be enough for a user to compute the delta and derive >>> speed. I'll think more about this since some devices may simplify that case if >>> the hardware is able to compute the speed for us. >>> >> >> I agree that it probably doesn't make sense to expect drivers to compute the >> speed. There isn't really a general way to do that works for an arbitrary >> speed. For example at high speeds, it is better to just look at the change >> in counts over a fixed interval rather than triggering a timestamp based on >> a certain number of counts. > > This is a good point. Depending on the resolution the user cares about, > they may be more interested in the speed over a short time interval > versus a long time interval. It doesn't seem practical to have the driver > try to handle all possible speed calculations when the user can decide > themselves how best to use the data. > >> I also don't think having a timestamp sysfs attribute would be very useful. >> To make it work at all, I think it would have to be implemented such that >> it returns the timestamp for the count that was most recently read via sysfs. >> And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/ >> timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not >> a negligible amount of time. > > This is a concern I've had as well. The sysfs interface is useful in > that it provides an intuitive and human-friendly way to expose data > about devices. But as you note, there is considerable overhead in the > amount of syscalls we have to make to interact with multiple attributes. > > One solution that may work is providing a character device interface in > addition to the sysfs interface. I believe that should reduce the > syscall overhead since a user can pass in a data structure with a > configuration defining what data/actions they want, and receive back > all data in a single syscall. Just toying with the idea here, but I've been thinking that it might work well to be able to mmap a char device to access a ring buffer. Then there should basically be no overhead at all from getting information from the kernel to userspace. > > I think concern over latency was one of the reasons the GPIO subsystem > gained a character device interface as well. It's an addition to the > Counter subsystem that is worth considering, but the possible downsides > to such an interface should also be investigated. > >> I noticed that several of the other counter drivers also register an IIO >> device. So this got me thinking that perhaps the counter subsystem should >> just be for configuring a counter device an then the IIO subsystem should >> be used for triggers and ring buffers. >> >> For the general case a counter device could have two possible triggers. >> One that triggers an interrupt after X counts and another that triggers >> with a period of T nanoseconds (or microseconds). Both triggers would add >> a count/timestamp pair to an IIO ring buffer. >> >> To fully reproduce our current methodology the first trigger would actually >> need two configurable settings, the count X that triggers every X counts and >> a watchdog time setting (using terminology from eQEP docs) that will also >> trigger if and only if the count does not change before the time has elapsed. >> Note, this is different from the other proposed time based trigger which >> would cause a trigger interrupt at a fixed period regardless of whether >> the count changed or not. > > The counter drivers in the kernel right now are registering IIO devices > in order to keep the preexisting (but now deprecated) IIO Counter > interface working for these devices -- some users may be using this > older interface so we don't want to remove it cold turkey. Regardless, > there's nothing the prevents incorporating the IIO interface with your > Counter drivers; in fact, in some circumstances it's better that you do > just that. > > The key idea to recognize is how the Counter subsystem differs from the > IIO subsystem on a conceptual level: the IIO subsystem provides an > interface for your device by describing it on a hardware level, whereas > the Counter subsystem provides an interface for your device by > describing it on a more abstract level. > > What I mean is that every interface interaction in the Counter subsystem > relates to the abstract concept of an ideal "counter device" (Counts, > Synapses, Signals); if a device functionality or data does not relate > directly to those ideal counter device components, then the Counter > subsystem isn't that right interface for it. > > For example, it makes sense to have an "enable" attribute or "present" > attribute, because these functionalities/data are directly related to > the Count, Synapse, and Signal components conceptually. However, in the > Counter subsystem you will likely not see something like the IIO > "in_voltageY_supply_raw" attribute -- not because that data is not > useful to know about for the operation of the counter device hardware, > but because it is outside the scope of the Counter subsystem paradigm > (i.e. it does not directly related to Counts, Synapses, or Signals). > As such, this would be a case where the counter driver should register > both a Counter device and IIO device, one to handle the counter device > on an abstract level while the other provides an interface for control > of the more specific hardware details. Makes sense. I that case, I don't see a need for an IIO device for the eQEP. > >> --- >> >> Thinking more generally though, I think what I would propose is adding a new >> component to the existing list of Count, Signal and Synapse. The new component >> could be called Event. Event would be more general than the trigger conditions >> I have just discussed. In addition to those two, it could be any event >> generated by the hardware, such as an error condition or a change in direction. >> >> Drivers could register an arbitrary number of events for each Count, so we >> would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few >> standard attributes, like "name" and "enable". Configurable events would need >> ext attributes to allow configuration. >> >> However, I see that there are already preset and error_noise "events" for >> count objects, so maybe we don't do the eventY thing and keep it flatter (or >> is the counter subsystem still considered in "staging" where breaking ABI >> changes could be made?). > > The components for handling events already exist in the Counter > interface paradigm: Signals and Synapses. Although, the Counter > subsystem is currently lacking the implementation (I still need to code > in support for interrupts and such), the paradigm itself supports the > concept of events and triggers. > > Recall that the Counter subsystem represents hardware via the > abstraction of an idealized "counter device". This is important to > understand because it means that Signals are not necessarily limited to > the physical wires of the hardware. To summarize the Counter interface > paradigm: > > * A Signal is a stream of data to be evaluated. > * A Synapse is a trigger condition based on the evaluation of the > data streams (i.e. the Signals). > * A Count is the accumulation of the effects of Synapses (i.e. the > triggers). > > As such, in order to represent an event, you would add in a Signal to > represent the stream of device events, and a Synapse defining the > specific event that will trigger the action. I'll give an example in > order to demonstrate what I mean. > > A simple clock can be conceptualize as a proper counter device: an > oscillation is a Signal, a rising edge from that oscillation line can be > the Synapse, and the current clock value is the Count. > > Count Synapse Signal > ----- ------- ------ > +---------------------+ > | Data: Clock ticks | Rising Edge _____________ > | Function: Increase | <------------- / Oscillation \ > | | _________________ > +---------------------+ > > Now, in order to represent your timestamping clock we need two Signals: > a simple clock and an event stream. The simple clock is the source of > the current clock ticks we will store, while the event stream provides > the rotation count register read notification that will trigger the > timestamp. > > Count Synapse Signal > ----- ------- ------ > +-------------------------------+ > | Data: Timestamp | None _______ > | Function: Current clock ticks | <------------ / Clock \ > | | ___________ > | | > | | Read event ________ > | | <------------ / Events \ > | | ____________ > +-------------------------------+ > > Note that in this case both Signals either do not exist in or are not > exposed by the hardware (maybe the simple clock is exposed, but it's not > necessary to be) -- they are meant to be abstract representations of the > components of the timestamp clock as an idealized "counter device". > > By organizing the timestamp clock in this way, we can control and > configure the components using the standard Counter interface: common > attributes such as "name", "preset", "enable", etc. can now be exposed > to users like every other counter device component. This way of looking at things makes very much sense. Thanks for the detailed explanation. > > In theory we can sleep on the timestamp count attribute read (or > character device equivalent if we go down that route), and be woken when > an event triggers updating the timestamp value. However, the current > Counter subsystem implementation is lacking the code for this so it > needs to be added to the core functionality first. > >> When thinking about what events would actually do when enabled though, it >> seems like we should be using IIO events and triggers (we have found reading >> sysfs attributes to be insufficient performance-wise). It seems like unnecessary >> work to reproduce all of this in the counter subsystem. Which makes me wonder if >> it would be better to have counter devices just be a different device type (i.e. >> different struct device_type for dev->type) in the IIO subsystem instead of >> creating a completely new subsystem. > > I plan on adding interrupt support for the 104-QUAD-8 counter driver > since this device has some useful interrupts on configured threshold > conditions and such, so having the ability to handle an event rather > than constantly read and loop is something I want to have in the Counter > subsystem. > > It's possible that I can reuse some code from the IIO subsystem, as > Jonathan pointed out, but overall I believe these should be separate > subsystems. From the reasons described above, the IIO subsystem and > Counter subsystem have different goals and thus different > implementations. I don't think that's a bad thing, and we can share code > in the few cases where the two may overlap. > > Regarding whether to use IIO events and triggers within the TI eQEP > counter driver, I think we should wait for a proper Counter subsystem > implementation to be added first. My fear is that we'll have a similar > situation as what happened with IIO_COUNT, where we'll have to keep a > IIO interface present with a newer Counter interface. If adding in event > support to the Counter subsystem will take too long, we can add this TI > eQEP driver as-is now and later add in the timestamp support. I don't think we need triggers anymore since I now better understand what a synapse does. --- In summary, this has been a very helpful discussion. Back the the patch series I have submitted, I think it still makes sense to merge it now as-is (barring any serious issues) and the additional functionality we have been discussing can be added in the future as the framework for it is developed.
This series adds device tree bindings and a new counter driver for the Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP). As mentioned in one of the commit messages, to start with, the driver only supports reading the current counter value and setting the min/max values. Other features can be added on an as-needed basis. The only other feature I am interested in is adding is getting time data in order to calculate the rotational speed of a motor. However, there probably needs to be a higher level discussion of how this can fit into the counter subsystem in general first. This series has been tested on a BeagleBone Blue with the following script: #!/usr/bin/env python3 from os import path from time import sleep COUNTER_PATH = '/sys/bus/counter/devices' COUNTERS = ['counter0', 'counter1', 'counter2'] COUNT0 = 'count0' COUNT = 'count' CEILING = 'ceiling' FLOOR = 'floor' ENABLE = 'enable' cnts = [] for c in COUNTERS: enable_path = path.join(COUNTER_PATH, c, COUNT0, ENABLE) with open(enable_path, 'w') as f: f.write('1') ceiling_path = path.join(COUNTER_PATH, c, COUNT0, CEILING) with open(ceiling_path, 'w') as f: f.write(str(0xffffffff)) cnt_path = path.join(COUNTER_PATH, c, COUNT0, COUNT) cnts.append(open(cnt_path, 'r')) while True: for c in cnts: c.seek(0) val = int(c.read()) if val >= 0x80000000: val -= 0x100000000 print(val, end=' ') print() sleep(1) David Lechner (4): dt-bindings: counter: new bindings for TI eQEP counter: new TI eQEP driver ARM: dts: am33xx: Add nodes for eQEP ARM: dts: am335x-boneblue: Enable eQEP .../devicetree/bindings/counter/ti-eqep.txt | 18 + MAINTAINERS | 6 + arch/arm/boot/dts/am335x-boneblue.dts | 54 +++ arch/arm/boot/dts/am33xx-l4.dtsi | 27 ++ drivers/counter/Kconfig | 12 + drivers/counter/Makefile | 1 + drivers/counter/ti-eqep.c | 381 ++++++++++++++++++ drivers/pwm/Kconfig | 2 +- 8 files changed, 500 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.txt create mode 100644 drivers/counter/ti-eqep.c