Encoder library modifications in UF2

[steverpi]

I use a standard and industrial rotary encoders in my RPI and Microchip projects and I am now slowly changing everything over to using the Pico W from Pimoroni, which I am finding is a most remarkable device. Could the encoder library in the UF2 be modified to be similar to other encoder routines. I will try to explain the changes and the reasons for them. Having looked at the encoder header source in uf2 1.20 onwards, the current encoder setup is:

Encoder::Encoder(PIO pio, uint sm, const pin_pair &pins, uint common_pin, Direction direction, float counts_per_rev, bool count_microsteps, uint16_t freq_divider)

1. The 'uint common_pin' setup is not needed as the common pin of an encoder really ought go to Gnd/0v. This setup is wasting a valuable and precious GPIO pin on the pico for no purpose whatsoever and also wastes a small amount of code space.

Presumably that common pin setup wss to be used to alter the polarity of the ‘common’ pin (by changing the pin from 0v to Vcc) so as to effectively alter the direction of rotation. If so, this pin change is not really needed as all that is needed to change direction is to swap over the ENC_A and ENC_B pin definitions, which alters the rotation direction from CW to ACW.

2. The ‘Direction direction’ setup is then not needed as the direction will have been decided by the ENC_A/B pin definitions and in any event the direction initial value cannot be re-entered and re-inited whilst running, so it is not needed. Again remove it to save code.

3. Typical industrial encoders have many microstep pulses per mechanical click (usually the more costly the encoder the more the number of microstep pulses per click, eg some have 4, 6, 8)

Remove the 'bool count_microsteps' setup and in its place change it to 'uint8_t microstep_divider' and in this setup you enter the number of microstep pulses which the particular encoder model produces per mechanical click (eg 4).

The encoder routine should read every microstep all of the time as normal and divide those microsteps by this entered setup value, eg 4. Output the integer count and this will then give a an encoder output count of 1 per mechanical click, rather than 4 counts as is the case at the moment.

This will allow any the encoders with 1 to 255 microsteps to be simply used. (and if you still wanted every microstep, irrespective of the mechanical clicks, just enter a value of 1 into this setup).

4. Keep an internal flag for rotation, CW or ACW. If the last rotation was say CW and the new rotation is also CW, output the counts as normal.

However if the rotation was say CW and the new rotation changes to ACW, do not output any change, ignore it and just alter this flag to ACW, ready for the next movement , which will presumeably be ACW, ie the same as last and so now output a change as normal

The reason for this change of direction trap to ignore the first instance of a change of direction is because in a critical application, if the encoder contacts encountered a glitch it could appear as a change of direction. This modification removes that happening and will only output when the last movement (whenever it occurred) was in the same direction.

Conclusion: if this library was changed a GPIO would be saved, any encoder with differing micro steps could be used in industrial projects and a potential glitch could be prevented.

[ZodiusInfuser]

Hi @steverpi

Thank you for your message. I'll do my best to address your comments below.

1. The 'uint common_pin' setup is not needed as the common pin of an encoder really ought go to Gnd/0v. This setup is wasting a valuable and precious GPIO pin on the pico for no purpose whatsoever and also wastes a small amount of code space.

1. The common_pin is an optional argument that by default is provided with PIN_UNUSED, so does not use an extra GPIO pin unless the user explicitly needs or wants that. You can see this check here: https://github.com/pimoroni/pimoroni-pico/blob/b82d16e8ae9b3501c65cdd0b4e0a4cb1c03b7df1/drivers/encoder/encoder.cpp#L153-L157

Presumably that common pin setup wss to be used to alter the polarity of the ‘common’ pin (by changing the pin from 0v to Vcc) so as to effectively alter the direction of rotation. If so, this pin change is not really needed as all that is needed to change direction is to swap over the ENC_A and ENC_B pin definitions, which alters the rotation direction from CW to ACW.

It is provided for those users who may wish to wire an encoder up a set of 3 pins in-line on a board without the need for wires, as this often will not have a GND sandwiched between two GPIOs. It is not to change the direction being read.

2. The ‘Direction direction’ setup is then not needed as the direction will have been decided by the ENC_A/B pin definitions and in any event the direction initial value cannot be re-entered and re-inited whilst running, so it is not needed. Again remove it to save code.

There are several reasons I can think of why changing the direction in software can be valuable. For one, that earlier soldering example. The main one though can be with motors with prewired encoders, where it is often likely that the direction the encoder reads does not match the direction of the motor's output shaft. Yes, swapping ENC_A and ENC_B pin definitions would achieve the same effect, but may no longer be technically accurate to the schematic.

3. Typical industrial encoders have many microstep pulses per mechanical click (usually the more costly the encoder the more the number of microstep pulses per click, eg some have 4, 6, 8)

Remove the 'bool count_microsteps' setup and in its place change it to 'uint8_t microstep_divider' and in this setup you enter the number of microstep pulses which the particular encoder model produces per mechanical click (eg 4).

The encoder routine should read every microstep all of the time as normal and divide those microsteps by this entered setup value, eg 4. Output the integer count and this will then give a an encoder output count of 1 per mechanical click, rather than 4 counts as is the case at the moment.

This will allow any the encoders with 1 to 255 microsteps to be simply used. (and if you still wanted every microstep, irrespective of the mechanical clicks, just enter a value of 1 into this setup).

This is an interesting thought, as I myself have encountered encoders with 2 and 4 microsteps per mechanical click. I am very curious what kinds of industrial encoders have 6, 8 or more microsteps per mechanical click though. Could you provide me with some links?

4. Keep an internal flag for rotation, CW or ACW. If the last rotation was say CW and the new rotation is also CW, output the counts as normal.

However if the rotation was say CW and the new rotation changes to ACW, do not output any change, ignore it and just alter this flag to ACW, ready for the next movement , which will presumeably be ACW, ie the same as last and so now output a change as normal

The reason for this change of direction trap to ignore the first instance of a change of direction is because in a critical application, if the encoder contacts encountered a glitch it could appear as a change of direction. This modification removes that happening and will only output when the last movement (whenever it occurred) was in the same direction.

I am curious what critical application would be using such an encoder with physical contacts that are susceptible to glitches as you describe. I would expect those cases to use optical or magnetic encoders with much cleaner transitions. I suspect this is just my limited knowledge of the encoder space though. Perhaps you can link me to the specific model of encoder you are referring to?

Conclusion: if this library was changed a GPIO would be saved, any encoder with differing micro steps could be used in industrial projects and a potential glitch could be prevented.

As mentioned, the extra GPIO is optional, and the differing microsteps is a beneficial addition. We do not sell any encoders that require that addition though, and without having hardware in hand to verify with I would not be confident in changing the code to support your use-case. You are most welcome to raise a pull-request with the modification though 🙂

[steverpi]

Hello

Thank you for your email, which sadly does not progress my requests as it appears to want me to justify raising what I thought were valid questions. I felt somewhat taken aback being asked to look for links to justify my questions - anyhow try looking at Bourns Industries, used in many aerospace applications and some of the original Broadcom units which used to be sold by RS and Farnell - I am not sure of their range now.

I am sorry that the routine cannot be looked at and enhanced, especially the microstep issue, which could easily be tested out by just altering the step value using any encoder for tests and seeing the count decrease accordingly. A specific encoder would not need to be purchased just for the test.... I am sure that you know what I mean.

Incidentally the glitch issue is meant as a preventative issue, which is always a good thing, whether of not the user can invest in higher priced opto/hall devices. It costs nothing and so I also commend looking at it.

Thank you for taking the time to write explaining the reasons why you feel the routine should stay as it is, which is actually excellent but could be enhanced as I respectively requested at no cost. Steve

From: ZodiusInfuser @.*** Sent: 17 August 2023 12:17 To: pimoroni/pimoroni-pico Cc: SteveA; Mention Subject: Re: [pimoroni/pimoroni-pico] Encoder library modifications in UF2 (Issue #794)

Hi @steverpi https://github.com/steverpi Thank you for your message. I'll do my best to address your comments below.

1. The 'uint common_pin' setup is not needed as the common pin of an encoder really ought go to Gnd/0v. This setup is wasting a valuable and precious GPIO pin on the pico for no purpose whatsoever and also wastes a small amount of code space.
2. The common_pin is an optional argument that by default is provided with PIN_UNUSED, so does not use an extra GPIO pin unless the user explicitly needs or wants that. You can see this check here: https://github.com/pimoroni/pimoroni-pico/blob/b82d16e8ae9b3501c65cdd0b4e0a4cb1c03b7df1/drivers/encoder/encoder.cpp#L153-L157 Presumably that common pin setup wss to be used to alter the polarity of the ‘common’ pin (by changing the pin from 0v to Vcc) so as to effectively alter the direction of rotation. If so, this pin change is not really needed as all that is needed to change direction is to swap over the ENC_A and ENC_B pin definitions, which alters the rotation direction from CW to ACW. It is provided for those users who may wish to wire an encoder up a set of 3 pins in-line on a board without the need for wires, as this often will not have a GND sandwiched between two GPIOs. It is not to change the direction being read.
3. The ‘Direction direction’ setup is then not needed as the direction will have been decided by the ENC_A/B pin definitions and in any event the direction initial value cannot be re-entered and re-inited whilst running, so it is not needed. Again remove it to save code. There are several reasons I can think of why changing the direction in software can be valuable. For one, that earlier soldering example. The main one though can be with motors with prewired encoders, where it is often likely that the direction the encoder reads does not match the direction of the motor's output shaft. Yes, swapping ENC_A and ENC_B pin definitions would achieve the same effect, but may no longer be technically accurate to the schematic.
4. Typical industrial encoders have many microstep pulses per mechanical click (usually the more costly the encoder the more the number of microstep pulses per click, eg some have 4, 6, 8) Remove the 'bool count_microsteps' setup and in its place change it to 'uint8_t microstep_divider' and in this setup you enter the number of microstep pulses which the particular encoder model produces per mechanical click (eg 4). The encoder routine should read every microstep all of the time as normal and divide those microsteps by this entered setup value, eg 4. Output the integer count and this will then give a an encoder output count of 1 per mechanical click, rather than 4 counts as is the case at the moment. This will allow any the encoders with 1 to 255 microsteps to be simply used. (and if you still wanted every microstep, irrespective of the mechanical clicks, just enter a value of 1 into this setup). This is an interesting thought, as I myself have encountered encoders with 2 and 4 microsteps per mechanical click. I am very curious what kinds of industrial encoders have 6, 8 or more microsteps per mechanical click though. Could you provide me with some links?
5. Keep an internal flag for rotation, CW or ACW. If the last rotation was say CW and the new rotation is also CW, output the counts as normal. However if the rotation was say CW and the new rotation changes to ACW, do not output any change, ignore it and just alter this flag to ACW, ready for the next movement , which will presumeably be ACW, ie the same as last and so now output a change as normal The reason for this change of direction trap to ignore the first instance of a change of direction is because in a critical application, if the encoder contacts encountered a glitch it could appear as a change of direction. This modification removes that happening and will only output when the last movement (whenever it occurred) was in the same direction. I am curious what critical application would be using such an encoder with physical contacts that are susceptible to glitches as you describe. I would expect those cases to use optical or magnetic encoders with much cleaner transitions. I suspect this is just my limited knowledge of the encoder space though. Perhaps you can link me to the specific model of encoder you are referring to? Conclusion: if this library was changed a GPIO would be saved, any encoder with differing micro steps could be used in industrial projects and a potential glitch could be prevented. As mentioned, the extra GPIO is optional, and the differing microsteps is a beneficial addition. We do not sell any encoders that require that addition though, and without having hardware in hand to verify with I would not be confident in changing the code to support your use-case. You are most welcome to raise a pull-request with the modification though 🙂 — Reply to this email directly, view it on GitHub https://github.com/pimoroni/pimoroni-pico/issues/794#issuecomment-1682106262 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AD2HF53CXFH6S4HG4ATV2GDXVX4RXANCNFSM6AAAAAAZUJ4MKU . You are receiving this because you were mentioned.https://github.com/notifications/beacon/AD2HF54B3PK7FEVCTHRAZE3XVX4RXA5CNFSM6AAAAAAZUJ4MKWWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTEILTZM.gifMessage ID: @.***>

[Gadgetoid]

Closing as stale/complete/wontfix. One of the above, anyway!