Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gpio] Refactor GPIO and Signal implementation #686

Merged
merged 4 commits into from
Sep 29, 2021

Conversation

salkinium
Copy link
Member

@salkinium salkinium commented Sep 15, 2021

The STM32 GPIO classes are manually generated, which create a lot of code and a lot of files. This is particularly noticable on devices with many pins, both calls to lbuild build and scons slow down significantly.

Instead I'm moving all the pin specific data into one long file, and using a templated GpioStatic class to specialize with this data at compile time. This shows significantly faster generation and compile times. The GpioConnector class has been adapted for the changes with more used of modern C++ TMP features. I've also reworked the GpioUnused implementation sightly as a specialization of the GpioStatic class.

I've also moved the STM32 external interrupt implementation into their own module with a new implementation based on modm::inplace_function. This makes a lot more sense than using EXTI via Gpios, since there are more event lines than just the GPIO ones. On AVRs the EXTI lines are coupled to the Gpios more strongly, so it stays like that.

  • STM32 implementation
  • STM32F1 implementation
  • AVR implementation
  • Merge all implementation into one template as done with GpioSet
  • Using more C++20 compile-time / template helpers?
  • Testing: on most boards that I could find.
  • AVR/STM32 GPIO documentation
  • New EXTI Implementation
  • EXTI documentation
  • More EXTI examples

@salkinium salkinium added this to the 2021q3 milestone Sep 15, 2021
@salkinium salkinium force-pushed the feature/fewer_files_gpio branch 4 times, most recently from 25f196f to e4370d0 Compare September 15, 2021 18:17
@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Sep 15, 2021
@salkinium
Copy link
Member Author

I did a simple test in each blink example:

rm -rf modm && time lbuild build && scons -c && time scons
Blink Example lbuild build scons
DISCO-F469 ~5.5s -> ~1.5s ~68s -> ~52s
NUCLEO-F103 ~2.2s -> ~1.3s ~13s -> ~12s
BLUE-PILL-F103 ~1.8s -> ~1.1s ~10s -> ~9s
DISCO-F769 ~5.3s -> ~1.5s ~18s -> ~16s
NUCLEO-H743 ~4.3s -> ~1.5s ~19s -> ~17s

While compile time does decrease a little, lbuild build is much much faster now. This is a good change!

@chris-durand
Copy link
Member

While compile time does decrease a little, lbuild build is much much faster now. This is a good change!

Nice! I will take a look at it later today.

@salkinium
Copy link
Member Author

I will take a look at it later today.

Maybe you also find a better way to encode the Gpio-peripheral-signal dependencies, dragging around the template< Peripheral _ > Signal thing everywhere is a bit dumb. I'll also take a look at it.

@chris-durand
Copy link
Member

Maybe you also find a better way to encode the Gpio-peripheral-signal dependencies, dragging around the template< Peripheral _ > Signal thing everywhere is a bit dumb. I'll also take a look at it.

I might have found a usable way to avoid the ugly template< template< Peripheral _ > class Signal > everywhere in the drivers.
My idea is to make the signal data plain types instead of templates and the connection data is encoded in a separate internal type template<typename Signal, Peripheral p> struct SignalConnection not exposed to users.

The gpio data would look like:

struct DataA0 {
        [...]
        // signals
	struct BitBang { using Data = DataA0; };
	struct Ch1 { using Data = DataA0; };
        [...]
	template<typename Signal, Peripheral p> struct SignalConnection;
};
[...]

// partial specialization for non-matching case, will always trigger static assert
template<Peripheral p> struct DataA0::SignalConnection<DataA0::Ch1, p> {
	static_assert(
		(p == Peripheral::Tim2),"GpioA0::Ch1 only connects to Tim2!");
};

template<> struct DataA0::SignalConnection<DataA0::Ch1, Peripheral::Tim2> {
	using Data = DataA0; static constexpr int8_t af = 2;
	static constexpr Gpio::Signal Signal = Gpio::Signal::Ch1;
};

I will upload a small proof-of-concept for STM32 on my fork as soon as the F1 compiles.

@chris-durand
Copy link
Member

I am wondering if there is a problem with the STM32F1 alternate function configuration of this refactored version. For F1 the unspecialized version of GpioConnector<>::connect() does nothing and there is no partial specialization for Peripheral::Spi2 when I generate the files for the Nucleo F103 examples. So it looks like it would not configure the alternate function mode for Spi2 when calling connect() from the driver.

@chris-durand
Copy link
Member

@salkinium Proof-of-concept for removing the signal template template parameter: chris-durand@727fe18

@salkinium
Copy link
Member Author

Yes, that's great, I'm going to cherry pick that! You're right about the F1, the connect for unspecialized peripherals must not be empty. I'll do some more testing.

@chris-durand
Copy link
Member

Yes, that's great, I'm going to cherry pick that! You're right about the F1, the connect for unspecialized peripherals must not be empty. I'll do some more testing.

It would need a one-line change in all the drivers with a connect method. I have only done it for the STM32 uart. Furthermore, the other platforms have to be adapted as well. Should I do it or will you?

@salkinium
Copy link
Member Author

I'll work on it, also redo the AVR implementation to match.

@salkinium
Copy link
Member Author

I've modified your prototype just a little to reduce data duplication, quite happy with this implementation. Kinda obvious in hindsight to use template specialization to code the triplet dependencies. Still very neat.
I'm on holiday until next week without hardware access, so this won't get merged until then anyways.

@salkinium salkinium force-pushed the feature/fewer_files_gpio branch 7 times, most recently from d77c1f5 to 3b8ce49 Compare September 24, 2021 17:03
@salkinium salkinium changed the title [gpio] Refactor STM32 GPIO for fewer files [gpio] Refactor GPIO and Signal implementation Sep 24, 2021
@salkinium salkinium force-pushed the feature/fewer_files_gpio branch 6 times, most recently from 823fdba to 6a3c45f Compare September 25, 2021 12:49
@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Sep 25, 2021
@salkinium salkinium force-pushed the feature/fewer_files_gpio branch 3 times, most recently from a1691f6 to 0581eb6 Compare September 26, 2021 18:23
@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Sep 26, 2021
@salkinium
Copy link
Member Author

This is ready to be merged, in case anyone wants to review this.

@rleh rleh self-requested a review September 27, 2021 09:42
@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Sep 27, 2021
@salkinium
Copy link
Member Author

Just FYI: Today is the last day for review, I'll be merging this tonight, since I'll be busy with the release tomorrow.

salkinium and others added 4 commits September 29, 2021 15:26
- Fewer files to generate and include
- Signals without templates

Co-authored-by: Christopher Durand <christopher.durand@rwth-aachen.de>
@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Sep 29, 2021
@salkinium
Copy link
Member Author

I'm merging this now.

@salkinium salkinium merged commit 30e24e6 into modm-io:develop Sep 29, 2021
@salkinium salkinium deleted the feature/fewer_files_gpio branch September 29, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced 🤯 ci:hal Triggers the exhaustive HAL compile CI jobs enhancement 🌈
Development

Successfully merging this pull request may close these issues.

2 participants