Messages in this thread | | | Subject | Re: [PATCH 0/4] new driver for TI eQEP | From | David Lechner <> | Date | Thu, 1 Aug 2019 12:37:56 -0500 |
| |
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.
| |