Conversation
|
rad! |
dhalbert
left a comment
dhalbert
left a comment
There was a problem hiding this comment.
I'm working on a rework of the state machine in the rotary encoder interrupt handler, but I'll approve it as is and submit another PR or issue later.
The refactoring is great! Really like removing all the #ifdef stuff and regularizing the file names and locations.
| self->eic_channel_b = pin_b->extint_channel; | ||
| self->pin_a = pin_a->pin; | ||
| self->pin_b = pin_b->pin; | ||
|
|
There was a problem hiding this comment.
Set self->position = 0 somewhere in the constructor.
| bool pin_b = gpio_get_pin_level(self->pin_b); | ||
|
|
||
| uint8_t this_state; | ||
| if (!pin_a && !pin_b) { |
There was a problem hiding this comment.
I think the current algorithm can still jitter the position count due to bounces. For example, consider ab = 00 going to 01. If b bounces it will oscillate between this_state being 1 and 2, which will cause self->position to be incremented and decremented on each bounce.
There are actually 16 state transitions to consider: ab->a'b', so 2^4 different combinations or states. Some of these are "illegal" and represent going too fast, etc.
I'm writing up a reimplementation that considers all these states. The best implementations I've found online that seems to do the right thing are https://github.com/buxtronix/arduino/blob/master/libraries/Rotary/Rotary.cpp and https://github.com/Erriez/ErriezRotaryEncoderFullStep/blob/master/src/RotaryFullStep.cpp.
The product 377 in the store does a full set of transitions between detents (on each detent both A and B are open).
| bool event_interrupt_overflow(uint8_t channel) { | ||
| bool overflow = false; | ||
| #ifdef SAMD21 | ||
| if (channel > 7) { |
There was a problem hiding this comment.
Off-by-1 error here for channels >= 8 (I had noticed this while debugging PDMIn. If, say, channel == 8, it will shift the channel number by 1 instead of by 0.
Change to:
if (channel >= 8) {
uint8_t value = 1 << (channel - 8);
Same issue at lines 156-157 and lines 171-172, but I can't leave comments there.
|
|
||
| //| .. method:: deinit() | ||
| //| | ||
| //| Deinitialises the IncrementalEncoder and releases any hardware resources for reuse. |
There was a problem hiding this comment.
Deinitialises -> Deinitializes
|
I think it would be convenient if IncrementalEncoder.position was also settable, so you could reset it to zero or whatever. |
|
Ok, updated as requested. I return early from |
|
Can't merge: Conflicting files: |
Ideally in the future they won't depend on ASF4 or MicroPython.
Always use current_tick when sub millisecond precision is required. Otherwise getting the ms/us to correspond is tricky.
|
Rebased and will wait for Travis to verify all boards compile. |
3 participants
The peripheral helpers have been reorganized to separate them out. Over time hopefully they can grow into a more useful library that doesn't depend on ASF4 or MicroPython.
This also makes ticks more atomic which should reduce errors with DHT sensors. @jerryneedell you may want to confirm.