henrikssn
commented
4 years ago Been there, done that. I have some very in-depth knowledge (sigh) about a number of issues with the signals API in general that needed to be solved:
Again, thanks for the detailed answer!
- You need a clear mapping of at least (Peripheral, Pin, Signal) to determine the correct bits to flip. In your case the Signal is given implicitly by argument position.
On SAM, this isn't actually enough. Here is how you would connect the (not-yet-implemented) SPI peripheral (with GPIO mappings made up but you get the point):
SpiMaster0::connect<GpioA1::Pad1, GpioA2::Pad2, GpioA3::Pad3>();How could the SPI interface possibly know which of these are MOSI, MISO and SCK given that SAM allows them to be attached to any pad in the relevant Sercom pin set?
- For the STM32F1 series, you can only remap groups of signals to overlapping groups of pins. Thus depending on the specific (Pin, Signal) group mapping at the callsite you must choose from multiple remap groups or static assert. Example from a generated file (
examples/blue_pill/blink/modm/src/modm/platform/gpio/connector_specialized.hpp:270):static_assert(/* check if group mapping exists */, "This pin set contains conflicting remap groups!\nAvailable groups for Tim2 are:\n" " Tim2 | 0 | 1 | 2 | 3 \n" "------|---------|---------|---------|---------\n" " A0 | Ch1,Etr | | Ch1,Etr | \n" " A1 | Ch2 | | Ch2 | \n" " A2 | Ch3 | Ch3 | | \n" " A3 | Ch4 | Ch4 | | \n" " A15 | | Ch1,Etr | | Ch1,Etr \n" " B3 | | Ch2 | | Ch2 \n" " B10 | | | Ch3 | Ch3 \n" " B11 | | | Ch4 | Ch4 \n" );
This also exists on SAM since each Sercom has two available pin sets attached to it.
- You need a variable number of connect pins with some default behavior. For example, you can use SPI to bit-bang WS2812 LEDs, but you only need MOSI for that. Or perhaps you only need to read something with only MISO and SCK. This makes it difficult to use fixed Signal argument positions with defaults:
template<class Sck = GpioUnused, class Mosi = GpioUnused, class Miso = GpioUnused > Spi::connect() {} Spi::connect<GpioUnused, GpioA1>(); // For Mosi only Spi::connect<GpioA0, GpioUnused, GpioA2>(); // for Sck, Miso only
I don't find this to be a big deal, but maybe it is a matter of taste.
- Due to lack of named arguments, you don't actually know what pin is connected to what signal, particularly with defaulted arguments. This is much clearer:
Spi::connect<GpioA1::Mosi>(); Spi::connect<GpioA0::Sck, GpioA2::Miso>();
On SAM, this example is Spi::connect<GpioA0::Pad4, GpioA2::Pad2>(), which is ambigous and hard to use (how do you know that it is Pad4 on A0 and not pad 1/2/3?)
- It's unclear what and how many Signals exactly you need to connect for your use-case For example the FSMC can be configured at runtime in different connection modes, making it quite difficult to do this without variadic templates. This is still type-safe, but not nothing was explicitly coded for this config:
GpioConnector<Peripheral::Fsmc, Board::display::fsmc::D0::D0, Board::display::fsmc::D1::D1, Board::display::fsmc::D2::D2, Board::display::fsmc::D3::D3, Board::display::fsmc::D4::D4, Board::display::fsmc::D5::D5, Board::display::fsmc::D6::D6, Board::display::fsmc::D7::D7, Board::display::fsmc::D8::D8, Board::display::fsmc::D9::D9, Board::display::fsmc::D10::D10, Board::display::fsmc::D11::D11, Board::display::fsmc::D12::D12, Board::display::fsmc::D13::D13, Board::display::fsmc::D14::D14, Board::display::fsmc::D15::D15, Board::display::fsmc::Noe::Noe, Board::display::fsmc::Nwe::Nwe, Board::display::fsmc::A18::A18 >::connect();
- Finally, you can define pin configuration defaults easily with our current API. For example, you can use UART with Tx or Rx or Tx+Rx, but this code works for all three cases, since GpioUnused is returned from
GetSignal<Tx>if the signal does not exist:template< template<Peripheral _> class... Signals > static void connect(Gpio::InputType InputTypeRx = Gpio::InputType::PullUp, Gpio::OutputType OutputTypeTx = Gpio::OutputType::PushPull) { using Connector = GpioConnector<Peripheral::{{ name }}, Signals...>; using Tx = typename Connector::template GetSignal< Gpio::Signal::Tx >; // Find Tx signal in group using Rx = typename Connector::template GetSignal< Gpio::Signal::Rx >; // Find Rx signal in group static_assert(((Connector::template IsValid<Tx> and Connector::template IsValid<Rx>) and sizeof...(Signals) == 2) or ((Connector::template IsValid<Tx> or Connector::template IsValid<Rx>) and sizeof...(Signals) == 1), "{{ name }}::connect() requires one Tx and/or one Rx signal!"); Tx::setOutput(OutputTypeTx); // Does nothing if GpioUnused Rx::setInput(InputTypeRx); // Does nothing if GpioUnused Connector::connect(); }
Won't this work for the same reason with your example above (Spi::connect<GpioUnused, GpioA1>())?
What would be useful is a slim HTML-only version of a pinout picker, where you use modm-devices data to configure the signal<->pin map visually and then generate the appropriate modm code. That would be much easier to prototype complex dependencies like SystemClock configurations.
There's already a curses-based CLI that does some of this: https://github.com/tgree/stm_layout
Maybe solving this in the API is indeed impossible, but this smells a bit bad API design to me (requiring an external tool just so figure out how to use the API).
And another point is that the the STM32 devices do not have a crossbar connect, thus you cannot connect every pin to any signal. So you have no choice but to look into the datasheet, or use the graphical CubeMX to configure your signals.
This is same for SAM, but usually only the groups are marked out (so you know which pins and the peripherals but not what signals you need to use for them). Note that the signals do have full crossbar support within them so it really doesn't matter which is which, from the programmers pov.
Maybe I am indeed trying to solve a SAM issue generically while it is a non-issue on all other platforms, maybe we just need another model specifically for SAM here (to remove ambiguity between signals and peripheral pin function)?
salkinium
This is a break-out from a discussion in #159
@henrikssn:
@salkinium: