[gpio] Refactor SAM signals API #482

henrikssn · 2020-09-24T20:52:37Z

Goals:

Compilaton errors are understandable (contain necessary information about the context that lead to the error).
Completely in C++ templates, allows peripherals to perform further validation.
Allows the user to select the intended peripheral signal (e.g. TX, SCK, MOSI, SDA)
Allows peripherals to take arguments in arbitrary order (my gift to @salkinium )
Deduces the pin function at compile time
Deduces the Peripheral pad at compile time (e.g. Sercom Pad)
As close to the current signals API as possible

Fixes #481.

salkinium · 2020-09-24T21:18:15Z

This looks great on first viewing! The STM32 API was still C++11 based, which didn't have variadic templates, so we generated everything with Python. I've always been busy fixing lbuild/modm-devices/tooling stuff that I don't have enough time to properly engage on the C++17 API front. There's a lot more to improve.

I'm still busy with TinyUSB and then I'd like to review/merge your OTA code, so I'll check this out, but probably not fast.

PS: Please fix your formatting style, or I'm going to have a hard time reviewing this.

henrikssn · 2020-09-25T16:08:02Z

This looks is great on first viewing! The STM32 API was still C++11 based, which didn't have variadic templates, so we generated everything with Python. I've always been busy fixing lbuild/modm-devices/tooling stuff that I don't have enough time to properly engage on the C++17 API front. There's a lot more to improve.

I'm still busy with TinyUSB and then I'd like to review/merge your OTA code, so I'll check this out, but probably not fast.

Yeah I learned a lot about C++17 while doing this, which was my main motivation. It believe it should be fairly straight-forward to make the STM32 API follow the pattern, but I'd like to make that a separate PR.

The OTA code is still WIP and not ready to merge, I would recommend you start here and then look at the fiber PR.

PS: Please fix your formatting style, or I'm going to have a hard time reviewing this.

Fixed, I also added the .clang-format file which I used so that I can stay more consistent in the future.

PS: How I dislike those hard tabs...

salkinium

I like this very, very much! It's so much cleaner and much faster to generate/compile than the STM32 solution, which generates 200+ files just for the GPIOs, which is just dumb.

I added a closer-to-modm-style clang format, but I guess we're going to have to use // clang-format off for some of the short function stuff. You still have to set hardtabs in your editor for the template stuff. Theres a .editorconfig for that purpose, it's supposed to be editor independent.

I would still like to see an alias for using GpioA00 = Gpio<A00>; for backwards compatiblity? Or perhaps just for STM32? I'm not sure I like having a A00 type floating around globally (due to using namespace modm::platform;), so perhaps it should be using GpioA00 = Gpio<GpioConfigA00>;?

salkinium · 2020-09-27T04:04:31Z

src/modm/board/feather_m0/board.hpp

-using LoggerDevice = modm::IODeviceWrapper< Uart0, modm::IOBuffer::BlockIfFull >;
-using Leds = SoftwareGpioPort< Led >;
+using LoggerDevice = modm::IODeviceWrapper<Uart0, modm::IOBuffer::BlockIfFull>;
+using Leds = Led;


This does not have the same semantics. Leds::write(uint32_t) accepts a bit vector, Led::write(bool) only accepts a single bit.

Yes, but a bit vector is implicitly convertible to a bool, so it should be ok in all cases.

Here is the line I am trying to make it work with:

modm/src/modm/board/board.cpp.in

Line 50 in 3072005

Board::Leds::write(1);

Is it expected that it only will blink the first led?

What is the benefit of removing the SoftwareGpioPort<> here?
I would like to keep it in order to have the same board.hpp files for further automation and testing in the future (#359).

It wasn't implemented on SAM properly and the new Gpio<> is drop-in compatible with a SoftwareGpioPort<> that only contains one port (which is the case for all SAM boards at the moment).

Maybe we can move the SoftwareGpioPort<> to a common library after stm32/avr supports the same signals API?

src/modm/board/feather_m0/board.hpp

src/modm/platform/gpio/sam/config.hpp.in

src/modm/platform/gpio/sam/pin.hpp

henrikssn

I like this very, very much! It's so much cleaner and much faster to generate/compile than the STM32 solution, which generates 200+ files just for the GPIOs, which is just dumb.

Thanks for the review!

I added a closer-to-modm-style clang format, but I guess we're going to have to use // clang-format off for some of the short function stuff. You still have to set hardtabs in your editor for the template stuff. Theres a .editorconfig for that purpose, it's supposed to be editor independent.

I disagree your clang-format is substantially closer to modm-style than the one I added. I force-pushed the branch back to where it originally was, in the future, please make changes in commits so that we can review the differences. I spent multiple hours aligning it with the modm style so it would be good to compare our efforts...

Also feel free to comment on the style in this PR, then we can see how we can fix that in the clang-format file.

I used this command to assess quality of the clang-format file: find src/modm | grep -e "\.$c\|h\|hpp\|cpp$\$" | xargs clang-format -n 2>&1 | wc -l (lower is better)

Your config: 143759
My config: 143800

Note that my config does allow the short functions, so no need for the // clang-format off.

I would still like to see an alias for using GpioA00 = Gpio<A00>; for backwards compatiblity? Or perhaps just for STM32?

I'm fine with adding the using declarations, but it should be a separate templated header file (which we could probably share between the platforms).

I'm not sure I like having a A00 type floating around globally (due to using namespace modm::platform;), so perhaps it should be using GpioA00 = Gpio<GpioConfigA00>;

How about PinA01 then? GpioConfigA00 is too long for my taste.

src/modm/board/feather_m0/board.hpp

salkinium · 2020-10-03T20:46:49Z

src/modm/platform/gpio/sam/config.hpp.in

+
+%% endfor
+
+} // namespace modm::platform


This file needs to be converted to tabs and the { put on a newline.

Braces done, I still need to figure out how to configure my editor (VSCode) for properly emitting tabs

I've added a .editorconfig to make GitHub render tabs as 4 spaces. Perhaps you can use the same mechanism. Something like this perhaps:
https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig

I tried it but it makes it impossible to edit the python files, as there we use spaces and python considers the difference important

Then we need to add a filter for python files to the config.

Try adding this to .editorconfig, that should make the tabs go away for the Python files.

[*.{lb,py}] indent_style = space indent_size = 4

examples/samd/interrupt/main.cpp

salkinium · 2020-10-03T21:15:12Z

I'm currently super busy with manufacturing 100 railway signals, so I'm AFK for a few weeks.
If you can fix the { on the newline, then I'm happy to merge this.

salkinium

Need to make the CI pass and rebase and squash onto develop please.

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp

src/modm/platform/i2c/bitbang/bitbang_i2c_master_impl.hpp

salkinium · 2020-10-05T08:36:31Z

Perhaps using clang format is ok on new files, but we're not reformatting entire swathes of modm, just because one line changes. I'm not wasting my free time fighting an autoformatter.

salkinium · 2020-10-05T08:40:39Z

I'm sorry I'm exploding at you, but these are exactly the same issues that I first encountered ~3 years ago when investigating using clang format for modm (during the porting from xpcc) and it's very frustating to see all the failing corner cases. I like the idea, but I'm going to have to deal with the fallout of this decision for many, many years going forward and I don't want to burn out on this project from preventable causes.

salkinium · 2020-10-05T08:42:27Z

And why did the STM32 CMSIS header and FreeRTOS submodules change?

salkinium · 2020-10-05T08:47:58Z

*challening the inner grumpy old maintainer* get off my lawn!1!!

henrikssn · 2020-10-05T08:56:57Z

Rebased + fixed CI

henrikssn · 2020-10-05T08:57:01Z

I'm sorry I'm exploding at you, but these are exactly the same issues that I first encountered ~3 years ago when investigating using clang format for modm (during the porting from xpcc) and it's very frustating to see all the failing corner cases. I like the idea, but I'm going to have to deal with the fallout of this decision for many, many years going forward and I don't want to burn out on this project from preventable causes.

If I look at the changes, there are more cases where the existing style is inconsistent with the modm style than there are cases where clang-format IMO is wrong.

For example, the contribution guide specifies no space after template keyword, yet that is the case in all of the bitbang implementations. Also, brace wrapping of if/else conditions is done very inconsistently (and this one isn't well defined in the style guide). At some point, you need to bite the apple and decide which one is actually correct.

Obviously, if the goal is to have inconsistent style everywhere, then we should not use clang format [grumpy new contributor mode on] but then I expect no more comments on the style of my commits... [/grumpy]

salkinium · 2020-10-05T09:37:17Z

if the goal is to have inconsistent style everywhere

The goals are in order:

Don't burn out the maintainer.
Have somewhat structured, reviewable PRs, so we don't burn out the maintainer.
Have somewhat consistent, readable code, so we don't burn out the maintainer.
Don't burn out the maintainer.

All are pretty subjective, but forcing me to search the diff in a reformatted file is not acceptable. I also contribute to other projects and I don't reformat entirely unrelated lines of code because I want my changes merged and not bikeshed about code formatting.

Our formatting guide is basically about quickly grokking code, by visually matching vertically scoped code elements, like most control structured and function declarations.

Use tabs, yes we hate it too, but it's too late to change.
Put braces in a new line unless it becomes an unreadable mess.
Put the function name on a newline unless it becomes an unreadable mess.
Limit yourself to <100 chars unless it becomes an unreadable mess.

This is a cooperation between you and me, not a negotiation. You adapt your style a little and I look the other way and then we can both be maximize our happiness. This has worked quite well for most contributors and it can work well for us too.

the contribution guide specifies no space after template keyword, yet that is the case in all of the bitbang implementations.

The bit-bang implementation is adapted from code from 2006 I believe. Much modm code is over a decade old. When we ported xpcc to modm, we experimented with clang-format to change the tabs to spaces, but it was just too many changes it would have double the repository size and made it impossible to use git blame for anything anymore.

Which is why using clang-format on purely added files is very acceptable to me, but let's not plough over the entire repo on principle.

henrikssn · 2020-10-05T21:37:09Z

I reverted the style changes to the bitbang* files and rebased/squashed against develop.

salkinium

Excellent.

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp

rleh

Nice work! 👍

I have commented a few remarks and questions inline.

.editorconfig

src/modm/board/feather_m0/board.hpp

rleh · 2020-10-05T23:14:11Z

src/modm/board/feather_m0/board.hpp

-using LoggerDevice = modm::IODeviceWrapper< Uart0, modm::IOBuffer::BlockIfFull >;
-using Leds = SoftwareGpioPort< Led >;
+using LoggerDevice = modm::IODeviceWrapper<Uart0, modm::IOBuffer::BlockIfFull>;
+using Leds = Led;


What is the benefit of removing the SoftwareGpioPort<> here?
I would like to keep it in order to have the same board.hpp files for further automation and testing in the future (#359).

src/modm/platform/gpio/sam/module.md

src/modm/platform/gpio/sam/unused.hpp

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp

src/modm/platform/uart/sam/uart.hpp.in

src/modm/platform/usb/sam/usb.hpp

henrikssn

Thanks for the review!

src/modm/board/feather_m0/board.hpp

henrikssn · 2020-10-06T12:11:35Z

src/modm/board/feather_m0/board.hpp

-using LoggerDevice = modm::IODeviceWrapper< Uart0, modm::IOBuffer::BlockIfFull >;
-using Leds = SoftwareGpioPort< Led >;
+using LoggerDevice = modm::IODeviceWrapper<Uart0, modm::IOBuffer::BlockIfFull>;
+using Leds = Led;


It wasn't implemented on SAM properly and the new Gpio<> is drop-in compatible with a SoftwareGpioPort<> that only contains one port (which is the case for all SAM boards at the moment).

Maybe we can move the SoftwareGpioPort<> to a common library after stm32/avr supports the same signals API?

src/modm/platform/gpio/sam/module.md

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp

src/modm/platform/usb/sam/usb.hpp

rleh · 2020-10-06T21:01:59Z

Merge @salkinium?

salkinium

Squashed and rebased, I'll merge once the CI passes.

salkinium · 2020-10-07T00:50:58Z

tools/scripts/clang-format.sh

+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#----------------------------------------------------------------------------
+
+git diff --name-only --diff-filter=A -C -M develop | grep -e "\.\(c\|h\|hpp\|cpp\)\$" | xargs clang-format -i "$@"


Added a filter so that only newly added files get formatted, otherwise I go crazy as discussed ;-P

Thanks! I realized this command is even better, as it reformats only changed lines: git clang-format upstream/develop

This was referenced Sep 24, 2020

[sam] Split signal name and index in GPIO signals modm-io/modm-devices#52

Merged

Discussion about Signals API #481

Closed

salkinium added advanced 🤯 enhancement 🌈 feature 🚧 lbuild 🏗 labels Sep 24, 2020

henrikssn force-pushed the feature/sam-gpio branch from c61f5f9 to c9de7d1 Compare September 25, 2020 15:58

salkinium force-pushed the feature/sam-gpio branch from c9de7d1 to a63aa57 Compare September 30, 2020 02:19

salkinium reviewed Sep 30, 2020

View reviewed changes

henrikssn force-pushed the feature/sam-gpio branch from a63aa57 to c9de7d1 Compare September 30, 2020 08:36

henrikssn commented Sep 30, 2020

View reviewed changes

salkinium reviewed Oct 3, 2020

View reviewed changes

salkinium approved these changes Oct 4, 2020

View reviewed changes

salkinium reviewed Oct 5, 2020

View reviewed changes

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp Show resolved Hide resolved

salkinium reviewed Oct 5, 2020

View reviewed changes

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp Outdated Show resolved Hide resolved

salkinium reviewed Oct 5, 2020

View reviewed changes

src/modm/platform/i2c/bitbang/bitbang_i2c_master_impl.hpp Outdated Show resolved Hide resolved

henrikssn force-pushed the feature/sam-gpio branch from 8003b41 to 44eed30 Compare October 5, 2020 08:48

henrikssn force-pushed the feature/sam-gpio branch from 44eed30 to e382657 Compare October 5, 2020 08:59

henrikssn force-pushed the feature/sam-gpio branch from b44c26d to 6382b44 Compare October 5, 2020 21:36

henrikssn force-pushed the feature/sam-gpio branch from 6382b44 to 4b1cd6d Compare October 5, 2020 21:37

salkinium approved these changes Oct 5, 2020

View reviewed changes

src/modm/platform/spi/bitbang/bitbang_spi_master_impl.hpp Show resolved Hide resolved

rleh requested changes Oct 5, 2020

View reviewed changes

henrikssn commented Oct 6, 2020

View reviewed changes

rleh approved these changes Oct 6, 2020

View reviewed changes

henrikssn added 2 commits October 7, 2020 01:59

[gpio] Refactor SAM signals API

dc56af2

[tools] Add clang-format style

debb430

salkinium force-pushed the feature/sam-gpio branch from 32e15b6 to debb430 Compare October 7, 2020 00:50

salkinium approved these changes Oct 7, 2020

View reviewed changes

salkinium merged commit debb430 into modm-io:develop Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gpio] Refactor SAM signals API #482

[gpio] Refactor SAM signals API #482

henrikssn commented Sep 24, 2020

salkinium commented Sep 24, 2020 •

edited

Loading

henrikssn commented Sep 25, 2020

salkinium left a comment •

edited

Loading

salkinium Sep 27, 2020

henrikssn Sep 30, 2020

rleh Oct 5, 2020

henrikssn Oct 6, 2020

henrikssn left a comment

salkinium Oct 3, 2020

henrikssn Oct 4, 2020

salkinium Oct 4, 2020

henrikssn Oct 5, 2020

salkinium Oct 5, 2020 •

edited

Loading

salkinium Oct 5, 2020

salkinium commented Oct 3, 2020

salkinium left a comment

salkinium commented Oct 5, 2020

salkinium commented Oct 5, 2020 •

edited

Loading

salkinium commented Oct 5, 2020 •

edited

Loading

salkinium commented Oct 5, 2020

henrikssn commented Oct 5, 2020

henrikssn commented Oct 5, 2020

salkinium commented Oct 5, 2020

henrikssn commented Oct 5, 2020

salkinium left a comment

rleh left a comment

rleh Oct 5, 2020

henrikssn left a comment

henrikssn Oct 6, 2020

rleh commented Oct 6, 2020

salkinium left a comment

salkinium Oct 7, 2020

henrikssn Oct 7, 2020

[gpio] Refactor SAM signals API #482

[gpio] Refactor SAM signals API #482

Conversation

henrikssn commented Sep 24, 2020

salkinium commented Sep 24, 2020 • edited Loading

henrikssn commented Sep 25, 2020

salkinium left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikssn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium commented Oct 3, 2020

salkinium left a comment

Choose a reason for hiding this comment

salkinium commented Oct 5, 2020

salkinium commented Oct 5, 2020 • edited Loading

salkinium commented Oct 5, 2020 • edited Loading

salkinium commented Oct 5, 2020

henrikssn commented Oct 5, 2020

henrikssn commented Oct 5, 2020

salkinium commented Oct 5, 2020

henrikssn commented Oct 5, 2020

salkinium left a comment

Choose a reason for hiding this comment

rleh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikssn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleh commented Oct 6, 2020

salkinium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium commented Sep 24, 2020 •

edited

Loading

salkinium left a comment •

edited

Loading

salkinium Oct 5, 2020 •

edited

Loading

salkinium commented Oct 5, 2020 •

edited

Loading

salkinium commented Oct 5, 2020 •

edited

Loading