[driver] Add best practice driver for soft graycode / quadrature decoding #580

TomSaw · 2021-03-17T09:29:39Z

Hi ✋, missed the gentle communication - here's my next 💢

Many modern MCUs have builtin quadrature-decoding support. However encoders alias rotary knobs kept being so usefull in small projects with small chips. Raphael commited the complement in 2019: modm/driver/encoder/encoder_output.hpp. So, let's don't discriminate the roots:

It's a port of well known, best practice solution: mikrocontroller.net/articles/Drehgeber.

Things to discus

I've redundantly added static_assert checking < UnderlyingType> in two methods: update() and getValue(). I found, this makes the compiler printing the assert-failure-string always on top. There may be a better technique.
getValue() could have been specialized for each value in < DIVISION >. Because of the difference of just 3 tiny lines, i found the it better using a switch-statement.
Alternative implemention of getValue() as functor: operator()(). To integrate the latest changes to a local variable v, one would type: v += encoder(); instead of v += encoder.getValue();
Following an example i've seen in another modm-module, i've put the credits to Peter Dannegger's ANSI-C foundation into encoder.lb ... but this may not be sufficient.

TomSaw · 2021-03-17T09:34:45Z

Example follows as soon as a future in the codebase is approved.

salkinium

Alternative implemention of getValue() as functor: operator()(). To integrate the latest changes to a local variable v, one would type: v += encoder(); instead of v += encoder.getValue();

Meh, we don't use operator() for anything else, so it would not be consistent with the rest of modm. I would actually prefer the getter, since it's explicit, otherwise it looks like a function call.

src/modm/driver/encoder/encoder_impl.hpp

src/modm/driver/encoder/encoder.hpp

src/modm/driver/encoder/encoder.lb

src/modm/driver/encoder/encoder_impl.hpp

TomSaw · 2021-03-17T11:14:51Z

Easy fixes done... let's generalize the Dividor

rleh · 2021-03-17T11:46:24Z

What is the use-case for this driver?
Typically I would wire an the A and B signals of an encoder to Ch1 and Ch2 of any STM32 "Advanced" or "General purpose" timer and let the timer hardware handle it.
Like here: https://github.com/roboterclubaachen/micro-motor/blob/master/src/hardware_rev2.hpp#L368-L390

With this driver you have to call update() at a very high frequency (>100kHz with some high-resolution encoders), otherwise some encoder ticks will be lost unnoticed.

TomSaw · 2021-03-17T11:51:39Z

What is the use-case for this driver?
Typically I would wire an the A and B signals of an encoder to Ch1 and Ch2 of any STM32 "Advanced" or "General purpose" timer and let the timer hardware handle it.
Like here: https://github.com/roboterclubaachen/micro-motor/blob/master/src/hardware_rev2.hpp#L368-L390

With this driver you have to call update() at a very high frequency (>100kHz with some high-resolution encoders), otherwise some encoder ticks will be lost unnoticed.

Hi Raphael. On small MCUs there's no such support. I've noted this in the opening message: "Many modern MCUs have builtin quadrature-decoding support. However encoders alias rotary knobs kept being so usefull in small projects with small chips."

Another case is, when you want an encoder on the fly, just for a quick debugging and no sufficient timer-pins are free.
Also there could be many encoders.. build some music-equipment f.e. where the hardware-decoders may not be sufficient.

rleh · 2021-03-17T11:52:18Z

I don't know how this works on other microcontrollers (AVR, SAM, ....).
Maybe some of them don't have hardware features like STM32, so this "bitbang" driver is useful there.

In my opinion this driver belongs to the "bitbang" category (like e.g. the spi.bitbang driver) and should be named accordingly.

rleh · 2021-03-17T11:53:15Z

On small MCUs there's no such support.

Which MCU are you using?

TomSaw · 2021-03-17T11:54:19Z

On small MCUs there's no such support.

Which MCU are you using?

Various ATmega... still. I like them somehow

TomSaw · 2021-03-17T11:55:10Z

I don't know how this works on other microcontrollers (AVR, SAM, ....).
Maybe some of them don't have hardware features like STM32, so this "bitbang" driver is useful there.

In my opinion this driver belongs to the "bitbang" category (like e.g. the spi.bitbang driver) and should be named accordingly.

That's a thing i would give in your hands!

Edit: Keeping it in :driver, encoder and encoder_output hang up together so the familiarity is exposed. encoders docs will tell you, that it's a Soft version and anyone who cares for the performance-drawbacks knows, there's hardware support.

rleh · 2021-03-17T12:07:07Z

I propose to call this driver modm:driver:encoder_input.bitbang.
Someday someone will take the effort to redesign the (horrible) STM32 timer HAL (see #275, #337) and will add a STM32 specific modm:driver:encoder_input driver...

Unsure if this driver belong to src/modm/driver/ or src/modm/platform/. What do you think @salkinium?

And maybe we should rename modm:driver:encoder.output to modm:driver:encoder_output.bitbang for consistency (but this would break backward compatibility)

src/modm/driver/encoder/encoder.hpp

src/modm/driver/encoder/encoder_impl.hpp

salkinium · 2021-03-17T13:04:15Z

Unsure if this driver belong to src/modm/driver/ or src/modm/platform/.

In modm/driver. I would even argue that the bitbang I2C and SPI classes belong in modm/driver, since they only depend on the GPIO interface, not even the implementation.

modm:driver:encoder_input.bitbang and odm:driver:encoder_output.bitbang

Yes, let's rename them.

TomSaw · 2021-03-17T13:25:58Z

Unsure if this driver belong to src/modm/driver/ or src/modm/platform/.

In modm/driver. I would even argue that the bitbang I2C and SPI classes belong in modm/driver, since they only depend on the GPIO interface, not even the implementation.

modm:driver:encoder_input.bitbang and odm:driver:encoder_output.bitbang

Yes, let's rename them.

modules are renamed.

How will the classes be named?
BitBangEncoderInput
BitBangEncoderOutput

Same for the preamble structs ... this should become encoder_[input/output] too, right?

salkinium · 2021-03-17T13:42:14Z

BitBangEncoder[In,Out]put, encoder_[input/output]

Yeah… kind of an eyesore, but it's consistent.

TomSaw · 2021-03-17T13:58:14Z

BitBangEncoder[In,Out]put, encoder_[input/output]

Yeah… kind of an eyesore, but it's consistent.

It entlightens one fair enough about the hidden gems from software-encoding.

TomSaw · 2021-03-20T07:17:57Z

The module-names are prefixed with the bitbang-classifier: encoder_[in/out]pu.bitbang
I found that filenames on the other hand are postfixed with the classifier: f.e. bitbang_i2c_master.hpp

For the encoder-files, i followed the postfix example

That's it from my side 😏 All implemented ✅ and tested ✅

salkinium

Very nice! Let's merge once the CI passes.

salkinium · 2021-03-21T19:12:24Z

src/modm/driver/encoder/bitbang_encoder_input.hpp

+				  "DeltaType is to small for POSTSCALER.");
+
+	using Phases = modm::platform::SoftwareGpioPort<SignalA, SignalB>;
+


Suggested change

static_assert(Phases::number_of_ports() == 1, "Signal A/B must be on the same GPIO port to prevent signal tearing!");

Otherwise the atomic read properties may not be true.

Very nice mechanism.. Damn i ♥ love ♥ the way you've implemented the low level stuff

I actually think it's just Phases::number_of_ports without a function call… I don't even remember the code I wrote.

Damn i ♥ love ♥ the way you've implemented the low level stuff

Thanks, half of the credit goes to C++17 with the relaxed constexpr rules:
#19 (comment)

I actually think it's just Phases::number_of_ports without a function call… I don't even remember the code I wrote.

It is and has already been concidered in the last commits ;)

Thanks, half of the credit goes to C++17 with the relaxed constexpr rules:

Thanks for the interresting background!

What's the magic spell to get these shift maps?

Here's the full example: https://github.com/modm-io/modm/blob/develop/examples/stm32f469_discovery/ports/main.cpp

src/modm/driver/encoder/bitbang_encoder_output_impl.hpp

TomSaw · 2021-03-22T06:44:52Z

If and how can i prevent in unittests-linux-generic ?

Please synchronize the modm documentation:
$ python3 tools/scripts/synchronize_docs.py
and then commit the results!
Exited with code exit status 1

rleh · 2021-03-22T12:10:00Z

python3 tools/scripts/synchronize_docs.py

TomSaw · 2021-03-22T12:14:02Z

python3 tools/scripts/synchronize_docs.py

Shame on me...

TomSaw · 2021-03-22T14:56:05Z

CI stucks with...

ERROR: In 'modm:platform:uart:2': Traceback (most recent call last): OSError: [Errno 28] No space left on device: '/root/project/build/test_all_5ilvyiug/modm/src/modm/platform/uart/uart_hal_2_impl.hpp'.

... and i wonder how - if any - i could have messed this up 🙄

salkinium · 2021-03-22T15:01:08Z

Ugh, CircleCI has made some recent changes in their free plan and now it's making us so much trouble. I'll try to rerun it, or we'll simply merge it without the F7 job. You're contributions didn't touch the HAL anyways.

TomSaw · 2021-03-22T15:41:54Z

You're #588 hunting for an alternative... No worries on my side :)

rleh · 2021-03-22T16:30:56Z

Why did you close this pull request? @TomSaw

salkinium · 2021-03-22T16:59:59Z

Thanks @TomSaw for sticking around for our review, it's a very beatiful driver now!

TomSaw · 2021-03-22T20:50:17Z

The pleasure is on my side. I've learned a lot and feels good to dive deeper into the catacombs of contribution.

TomSaw · 2021-03-22T20:56:13Z

Excuse the little disasters due to my github-newbieness @rleh , do you mean, it was a bad thing to delete the encoder branch after it has been merged into develop?

salkinium · 2021-03-22T21:04:15Z

The pleasure is on my side. I've learned a lot and feels good to go deeper into the catacombs of contribution.

That's good to hear, because I was worried we scared you off by descending on you like a plague of judgemental locusts…

after it has been merged into develop?

Emphasis on AFTER 😜 I think you mistook my force push to your encoder branch with a merge into develop, but I wanted to check the CI after the rebase, which was good, because I messed it up and had to fix it.

TomSaw · 2021-03-23T09:39:55Z

I was worried we scared you off by descending on you like a plague of judgemental locusts

I think any generic concept benefits from perfection. Forget the Pareto-principle here: The expensive fine-works pay out squared^squared^squared when you later build applications! out of the generic puzzle-pieces.

I also find lot's of satisfaction, making one thing perfect rather than going for quantity. You have my fullest respect when judging my lines cause it's for the good! - Amen

TomSaw force-pushed the encoder branch from d451081 to 9e63605 Compare March 17, 2021 10:07

salkinium requested changes Mar 17, 2021

View reviewed changes

TomSaw force-pushed the encoder branch from 3017e9a to 73b7ad0 Compare March 17, 2021 11:11

salkinium reviewed Mar 17, 2021

View reviewed changes

src/modm/driver/encoder/encoder_impl.hpp Outdated Show resolved Hide resolved

TomSaw changed the title ~~[driver] Add best practice driver for soft graycode / quadrature decoding~~ [driver] Add best practice software driver for soft graycode / quadrature decoding Mar 17, 2021

TomSaw changed the title ~~[driver] Add best practice software driver for soft graycode / quadrature decoding~~ [driver] Add best practice driver for soft graycode / quadrature decoding Mar 17, 2021

chris-durand requested changes Mar 17, 2021

View reviewed changes

TomSaw commented Mar 17, 2021

View reviewed changes

src/modm/driver/encoder/encoder_impl.hpp Outdated Show resolved Hide resolved

salkinium reviewed Mar 17, 2021

View reviewed changes

src/modm/driver/encoder/encoder_impl.hpp Outdated Show resolved Hide resolved

salkinium added example 🔑 feature 🚧 labels Mar 17, 2021

TomSaw force-pushed the encoder branch from 0c151ff to 47098b9 Compare March 17, 2021 13:31

TomSaw force-pushed the encoder branch 2 times, most recently from 38ca7a3 to 65fc036 Compare March 17, 2021 13:53

TomSaw force-pushed the encoder branch 3 times, most recently from 83eb468 to ce2b908 Compare March 17, 2021 18:26

TomSaw force-pushed the encoder branch 2 times, most recently from aa02245 to 0b91ea0 Compare March 20, 2021 06:45

TomSaw requested review from salkinium and chris-durand March 21, 2021 19:06

salkinium approved these changes Mar 21, 2021

View reviewed changes

TomSaw force-pushed the encoder branch 2 times, most recently from 8a379a5 to e6ffb6a Compare March 21, 2021 19:34

chris-durand approved these changes Mar 22, 2021

View reviewed changes

TomSaw force-pushed the encoder branch from c47bb14 to a769ad5 Compare March 22, 2021 06:21

[driver] Rename EncoderOutput to BitBangEncoderOutput

d7ecd88

TomSaw added 2 commits March 22, 2021 16:50

[driver] Add BitBangEncoderInput driver

9f89a3c

[examples] Add BitBangEncoderInput example on AVR

3752847

salkinium force-pushed the encoder branch from b368bae to 3752847 Compare March 22, 2021 15:53

TomSaw closed this Mar 22, 2021

TomSaw deleted the encoder branch March 22, 2021 16:27

rleh approved these changes Mar 22, 2021

View reviewed changes

salkinium mentioned this pull request Mar 22, 2021

Add BitBangEncoderInput implementation via #580 #590

Merged

		"DeltaType is to small for POSTSCALER.");

		using Phases = modm::platform::SoftwareGpioPort<SignalA, SignalB>;


	static_assert(Phases::number_of_ports() == 1, "Signal A/B must be on the same GPIO port to prevent signal tearing!");

[driver] Add best practice driver for soft graycode / quadrature decoding #580

[driver] Add best practice driver for soft graycode / quadrature decoding #580

Conversation

TomSaw commented Mar 17, 2021 • edited Loading

Things to discus

TomSaw commented Mar 17, 2021

salkinium left a comment

Choose a reason for hiding this comment

TomSaw commented Mar 17, 2021

rleh commented Mar 17, 2021

TomSaw commented Mar 17, 2021 • edited Loading

rleh commented Mar 17, 2021

rleh commented Mar 17, 2021

TomSaw commented Mar 17, 2021 • edited Loading

TomSaw commented Mar 17, 2021 • edited Loading

rleh commented Mar 17, 2021

salkinium commented Mar 17, 2021

TomSaw commented Mar 17, 2021 • edited Loading

salkinium commented Mar 17, 2021

TomSaw commented Mar 17, 2021

TomSaw commented Mar 20, 2021

salkinium left a comment

Choose a reason for hiding this comment

salkinium Mar 21, 2021

Choose a reason for hiding this comment

TomSaw Mar 21, 2021

Choose a reason for hiding this comment

salkinium Mar 21, 2021

Choose a reason for hiding this comment

salkinium Mar 21, 2021

Choose a reason for hiding this comment

TomSaw Mar 22, 2021

Choose a reason for hiding this comment

TomSaw Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

TomSaw Mar 22, 2021

Choose a reason for hiding this comment

salkinium Mar 22, 2021

Choose a reason for hiding this comment

TomSaw commented Mar 22, 2021 • edited Loading

rleh commented Mar 22, 2021

TomSaw commented Mar 22, 2021

TomSaw commented Mar 22, 2021

salkinium commented Mar 22, 2021

TomSaw commented Mar 22, 2021 • edited Loading

rleh commented Mar 22, 2021

salkinium commented Mar 22, 2021

TomSaw commented Mar 22, 2021 • edited Loading

TomSaw commented Mar 22, 2021 • edited Loading

salkinium commented Mar 22, 2021

TomSaw commented Mar 23, 2021

TomSaw commented Mar 17, 2021 •

edited

Loading

TomSaw commented Mar 17, 2021 •

edited

Loading

TomSaw commented Mar 17, 2021 •

edited

Loading

TomSaw commented Mar 17, 2021 •

edited

Loading

TomSaw commented Mar 17, 2021 •

edited

Loading

TomSaw Mar 22, 2021 •

edited

Loading

TomSaw commented Mar 22, 2021 •

edited

Loading

TomSaw commented Mar 22, 2021 •

edited

Loading

TomSaw commented Mar 22, 2021 •

edited

Loading

TomSaw commented Mar 22, 2021 •

edited

Loading