docs\machine: Add Counter and Encoder classes. #8072

IhorNehrutsa · 2021-12-09T23:07:25Z

This PR is based on the discussion in #6639.
It describes the common hardware APIs for Counter and Encoder classes.

Link
:ref:ESP32 <esp32_machine.Counter>
is located in
ESP32: New hardware PCNT Counter() and Encoder(). #6639

Link
:ref:MIMXRT <mimxrt_machine.Counter>
is located in
mimxrt: Initial documentation for the mimxrt port. #7494

Moved to docs\machine: Add Counter and Encoder classes. micropython#8072 micropython#8072

robert-hh · 2021-12-10T08:05:07Z

Thank you for the documentation of the minimal API of the Encoder/Counter class. A few notes:

in the way you document it, the arguments pulse, phase_a and phase_b are keyword arguments as well, even if they are mandatory. So the sentence would be. "Further keyword arguments". I was codsidering a proper name for the counter input pin, and called it 'input'.
x124 cannot be supported by all ports. So you should make clear, that there are port specific features, like x124.
The scale argument and position() method are obsolete. Especially when considering callbacks for a matching counter value you cannot make it reliable. So this kind of scaling should be left to user code.

IhorNehrutsa · 2021-12-10T08:23:53Z

@robert-hh

I was codsidering a proper name for the Counter() input pin, and called it 'input'.

I saw that you names the Counter() input pin as 'input'. But I think that there are many inputs and 'pulse' represents this input signal more accurately/definitely, like phase_a and phase_b better then input_a and input_b for Encoder().

robert-hh · 2021-12-10T10:10:41Z

I do not like pulse. How about "signal"?
Also about X124: You should not mention a specific port, since that is never exhaustive. It is sufficient to put x124 into the ESP32 port specific section, like position and scale, and document x4 as the default behavior.

IhorNehrutsa · 2021-12-10T10:32:12Z

About Counter() input.
What is the Counter() counting? Pulses, impulses, inputs or signals?
I mean that pulses==impulses.

For example Electronic Impulse Counter

jonathanhogg · 2021-12-10T12:38:40Z

Thoughts:

I'd add UP and DOWN constants and say that direction should be set to one of these – don't bake magic numbers into the API;
Shouldn't Counter have an edge keyword argument? Again, with matching RISING and FALLING constants for the values;
I'd argue for filter_ns so that the units are explicit – c.f. new read_uv() ADC design;
I agree about ditching scale and position() – it's just not enough utility over doing it in user code;
Also, yes, lose x124 – it's too port specific.

I'd just say that individual ports may differ on what form of quadrature decoding they do, or may allow this to be configured, but x4 is the preferred default.

Don't think pseudo-code is needed for the value() method – the documentation is pretty clear. Are we making returning the old value when setting part of the API? If so, that needs documenting and explaining that ports should try to do this atomically if they can.

I tried to find comparisons for naming of the counter pin argument and the closest I could find is the dest keyword argument to PWM(); so I wonder if src might be a good name for this argument? I would expect that most ports will make it the first argument and allow positional usage anyway.

Also, I think this should probably be src=-1 in the constructor overview. src=Pin() makes no sense from a Python point of view. I'm open to arguments for src=None instead, but -1 seems to be pretty common in the rest of the codebase.

There's a few spelling and wording things that need fixing, but those can wait for a final pass.

jonathanhogg · 2021-12-10T12:47:25Z

On a side note: another reason I disagree with adding position() is that it just isn't. For proper position support you need (at least) to configure the number of detents in the encoder and use an index pulse. Really, position is always best measured with an optical shaft encoder.

It's best to step away from a half-solution and leave the user to understand their particular use-case and code appropriately for it.

robert-hh · 2021-12-10T14:15:11Z

@jonathanhogg
The edge option for the counter is not supported by the MIMXRT hardware, for whatever reason. Otherwise I would have it implemented. Other signal input like that for Index and Home allow to select the egde, but I've set that to RISING.

filter_ns is a good suggestion.

About the name of the Pin for the counter, I still miss a real catching word for that. I had chosen 'input' as a device view of the interface, @IhorNehrutsa has chosen 'pulse' as a signal type view, but none of that is as obvious as phase_a and phase_b for the Encoder.

jonathanhogg · 2021-12-10T14:37:25Z

@jonathanhogg The edge option for the counter is not supported by the MIMXRT hardware, for whatever reason. Otherwise I would have it implemented. Other signal input like that for Index and Home allow to select the egde, but I've set that to RISING.

How curious! Does the i.MX RT provide a way to invert the logic of an input that would be equivalent?

Otherwise, the question is whether edge should still be part of the common API, it's just that the mimxrt port doesn't support values other than RISING, or whether to leave it out? It feels like such an obvious thing for counters to support…

About the name of the Pin for the counter, I still miss a real catching word for that. I had chosen 'input' as a device view of the interface, @IhorNehrutsa has chosen 'pulse' as a signal type view, but none of that is as obvious as phase_a and phase_b for the Encoder.

Yeah, I don't think there are any options that I find fully satisfying. I do think that talking about the "source" or "input" of a counter makes more sense than using pulse.

Looking through the MicroPython code, equivalent arguments are normally just called pin, but I dislike this on the basis that it limits the API: a counter might be hooked to an internal clock (as in the STM32 implementation of hardware timers). I think for this reason, I marginally prefer src over input as it feels more open.

robert-hh · 2021-12-10T15:08:19Z

How curious! Does the i.MX RT provide a way to invert the logic of an input that would be equivalent?

There is no simple invert bit on the GPIO input section. There is a so-called AIO-Logic module, which can And/Or/Invert several internal signals of the device, which are routed through the crossbar matrix. This crossbar is actually use to route the pad signals to the encoder module. But I'm not sure whether that could be used. The first step would be to tell, which egde is the active one at the moment.
Edit: it's the rising edge. But I'm not sure how important that is. Finally, full cycles count.

IhorNehrutsa · 2021-12-10T15:51:46Z

Further keyword arguments is still incomprehensible to me.

jonathanhogg · 2021-12-10T16:10:43Z

The Encoder constructor should probably be:

Encoder(id, /, phase_a=-1, phase_b=-1, *, filter_ns=0)

However, MicroPython does not support the / positional-only argument marker (although the C implementation supports positional-only arguments). So the closest you can get in the docs summary is:

Encoder(id, phase_a=-1, phase_b=-1, *, filter_ns=0)

I'm not sure it's even necessary to make a distinction and refer to "keyword arguments" – one only needs to refer to them by name and explain what they do. As noted before, it's not even necessary for a port to require the phase_a and phase_b parameters since they may have defaults or be hardwired to particular pins.

jonathanhogg · 2021-12-10T16:21:08Z

[Personally I'd prefer:

Encoder(id, *, phase_a, phase_b, filter_ns)
Encoder.init(*, phase_a, phase_b, filter_ns)

and make everything other than id keyword-only, but I recognise that's not the popular choice 😉.]

robert-hh · 2021-12-10T16:21:18Z

Encoder(id, phase_a=-1, phase_b=-1, *, filter_ns=0)

This phase_a=-1 looks more than confusing, especially since it is not a valid setting. It does not explain anything to me. Using phase_a=Pin(x) is much more intuitive, or phase_a=pin_spec.

jonathanhogg · 2021-12-10T16:26:23Z

Unfortunately, phase_a=Pin(x) is not a valid Python method declaration and this form isn't present in any other documentation in MicroPython.

-1 is just a suggestion based on my knowledge of MicroPython internals, None would be equally valid and might read better from a documentation perspective.

The docs honestly become simpler if you just call them keyword-only arguments as you can just name them and then explain what each should be below.

Something like the documentation of the recently-added machine.I2S would be a good reference.

jonathanhogg · 2021-12-10T16:29:24Z

In terms of "validity" of a particular value one should consider that a port may want to support something other than a Pin object – such as a pin number, name or some constant specifying one from a limited range of hardware-allowed options. There's a fair bit of relaxed usage here across the different ports currently.

robert-hh · 2021-12-10T16:42:04Z

For instance the MIMXRT port allows to use a number at places, where Pin is expected.
So: Maybe we can show them in the header line as keyword parameters and explain in the text, that a) the keyword can be omitted, and b) that the parameters are required in the constructor.

jonathanhogg · 2021-12-10T16:47:45Z

I wouldn't even say that they are required, since a port may not necessarily require them.

Given this is just the documentation of the minimal API, it'd be safe to say that they are keyword arguments that should at least accept a Pin object but may support other things for a particular port or may be optional if they can be inferred from the hardware unit.

You could then make these arguments positional and required in your implementation without breaking the API, providing they can still be specified by keyword.

robert-hh · 2021-12-10T17:22:16Z

You could then make these arguments positional and required in your implementation without breaking the API, providing they can still be specified by keyword.

That's how it is implemented for the MIMXRT port. They have to be given at least in the constructor, as keyword parameter or positional.

IhorNehrutsa · 2021-12-10T17:56:20Z

I don't like Counter() input again.

Counter() object has two inputs.
Why do we call one of them the 'direction' (which means the direction input), but the other - the 'input' (which means the pulse(impulse) input) ???

I don't see any logic. :((

phase_a and phase_b also inputs and sources. Why not input_a or src_a?

EDITED: I withdraw the claim

IhorNehrutsa · 2021-12-11T08:20:07Z

@jonathanhogg

There's a few spelling and wording things that need fixing, but those can wait for a final pass.

Please review.

robert-hh · 2021-12-11T08:57:49Z

EDITED: I withdraw the claim

Did you switch to "input" for the counter input Pin?

Please review (spellig).

What I usually do is use at least the spell check of a word processor. That fixes at least some of the error.

robert-hh · 2021-12-11T09:22:15Z

I added suspend() and resume() to the MIMXRT port as well, and switched back to "input", like documented here. Suspend() and Resume() may not be needed often, but implementing them was simple and short, and creates not further trouble, like spurious counts.

IhorNehrutsa · 2021-12-11T13:11:32Z

The input again

robert-hh · 2021-12-11T13:40:00Z

Thanks. I switched back as well.

jonathanhogg · 2021-12-11T16:24:16Z

Still liking src for the argument name… It feels a lost closer to input than pulse to me, is fairly common industry parlance (e.g., "clock source", "voltage source", etc.) and matches the dest argument to PWM().

robert-hh · 2021-12-11T19:34:27Z

It's running in circles. I had changed 'input' to 'src', just to notice that @IhorNehrutsa had changed 'pulse' to 'input'. So I changed back 'src' to 'input', just to see that 'input' was changed to 'pulse' again. So I'll wait for a while now.
I implemented a resume/suspend method pair. Later I saw that @IhorNehrutsa had called that resume/pause. But I had chosen 'suspend' without much thinking as the complementary term for resume, just like other term pairs as start/stop, black/white, or heaven/hell.

jonathanhogg · 2021-12-11T20:21:15Z

That's why we should be agreeing on the naming here in this documentation PR first.

I think maybe I'd lean towards pause() and resume() – it's familiar from playing media. I mainly see "suspend" used when referring to saving state and stopping something (like a laptop). I used stop() and start() in my code, but I can see that this might suggest that the method resets the counter.

robert-hh · 2021-12-11T20:33:07Z

Well, suspend saves the state, and resume picks it up at that point.

This is the MIMXRT specific version, compliant to the documentation of PR micropython#8072 in the basic methods. Signed-off-by: robert-hh <robert@hammelrath.com>

Add thin Python shims around `esp32.PCNT` to implement the `machine.Counter` and `machine.Encoder` classes (as per micropython#8072 API).