Add espressif rotaryio divisor support.

[domdfcoding]

Follow up to #5468, which don't include espressif support.

I have tested this on a QT Py ESP32-S2 with this type of rotary encoder, which requires divisor=1. I've also tested with divisor=2 and divisor=4, and a pulse is registered every 2 and 4 detents respectively as expected.

I had to change the order in shared-bindings/rotaryio/IncrementalEncoder.c as the divisor value needs to be known before configuring the peripheral. I don't think this will cause issues for other ports as the set_divisor function only sets a value on a struct, so the order of operations shouldn't matter.

[dhalbert]

I had to change the order in shared-bindings/rotaryio/IncrementalEncoder.c as the divisor value needs to be known before configuring the peripheral. I don't think this will cause issues for other ports as the set_divisor function only sets a value on a struct, so the order of operations shouldn't matter.

Rather than change the order, let's add the divisor argument to _construct, and change the other ports to handle that. Then we are not dependent on the order happening to work for all ports.

[domdfcoding]

I've realised the divisor property can be changed after creating the IncrementalEncoder object, but that would require reconfiguring the PCNT peripheral. I don't know why you'd want to change the property, so I've made it raise an error if the user tries to do that.

[dhalbert]

We might consider making that attribute read-only, since as you say, it's not clear why you'd want to change it. We could make it read-only just on espressif, but we try to avoid chip-specific idiosyncrasies if they are avoidable. This is an incompatible API change so we we might make it read-only on espressif now and then read-only on all ports in 8.0.0.

@jepler do you have an opinion about this?

[jepler]

I don't mind if you do this. I can't think of any super plausible reasons to change it at runtime (but https://hackaday.com/2022/03/13/haptic-smart-knob-does-several-jobs/ is cool! and probably not something you'd implement in CircuitPython)

The alternative is to always use the "count every quadrature transition" mode and slightly refactor shared_module_softencoder_state_update into two parts: the one that takes care of acting on transitions, and the one that takes care of maintaining the "count" and "sub_count".

It's worth noting there IS a slight bug in that code if divisor is changed dynamically: Suppose that initially divisor is 4 and sub_count is 3. When divisor is changed to 1, what should happen? Well, I don't know 😁 but what will happen immediately is NOTHING. Then, whether the next transition is a +1 or a -1, sub_count >= divisor will be true (sub_count being 2 or 4) and position will increase by 1. That seems a weird outcome.

[dhalbert]

@domdfcoding you pushed 319ca85 after our discussions of making divisor read-only, etc. What are your current thoughts?

[domdfcoding]

Sorry @dhalbert, I thought I had replied.

@jepler's suggestion to "count every quadrature transition" was the missing piece. By always configuring the second PCNT channel and counting every transition the code becomes much simpler, and allows the divisor attribute to be changed on the fly. The position is then just the internal transition count divided by the divisor.

The same approach could be used with the other ports, which would mean position would update immediately when divisor is changed.

I did find a bug in my current code if the position attribute is changed by the user, where the position would be wrong if the divisor changed. That's fixed in my latest commit, although it doesn't work correctly on other ports.

[dhalbert]

Thanks for the reply. Does it still make sense to pass the initial divisor in? I think that makes sense, because then you don't have to worry about the order of the calls.

[domdfcoding]

With the current code the constructor doesn't need to know the divisor anymore, so the order is back how it was before this PR.

[jepler]

I think it depends what you think should happen when the divisor is changed.

Given that I don't know the real world reason for dynamically changing the encoder divisor, I don't feel confident stating how it should work.

I see three possibilities:

• document that the position may change arbitrarily when changing divisor, the code can reset position to 0 if it wants
• make new movement relative to the old position, applying any pending sub-counts according to the new divisor
• multiply or divide the position according to the change in divisor, probably also accounting for sub-counts

Again, given that there's no particular scenario that we have in mind for changing this dynamically, I would probably take the simplest alternative: position is free to jump when changing divisor, the code can set the value (E.g., reset to zero) if it needs to.

[jepler]

If this passes your testing with divisors of 1 and 4, it's good enough for me. I didn't do any testing on HW myself.