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] Runtime GPIO with virtual interface #631

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

salkinium
Copy link
Member

This is a research PR to understand what is needed for a good runtime GPIO interface.

  • modm::Gpio with a minimal virtual interface.
  • modm::GpioPort with a minimal virtual interface.
  • New configure method for unified IO configuration.
    • Should be available also for static-type Gpio classes
    • Should be available also for static-type Port classes
    • constexpr validator of config features per static-type class
  • modm::platform::RtGpio runtime class
    • should be modm::platform::Gpio unless that causes ambiguity
  • Constructor from static-type Gpio class to platform runtime class.
    • Constructor from static-type Gpio class to virtual runtime class?
  • Concepts to replace modm::GpioInput, modm::GpioOutput and modm::GpioIO with better names
  • GpioUnused Implementation
  • Throughly tested
  • Implemented for all platforms
    • STM32
    • AVR
    • SAM
    • Hosted
  • Bonus: Reduce the amount of files/code generated by using more C++ template/constexpr magic instead of Jinja2. The lbuild and compile time improvements will be good!

The new configure(Config_t) method allows configuring the GPIO in it's entirety with one 32-bit value (perhaps 16-bit on AVR). It's designed so that only the Flags that you put in get changed, so you can incrementally change the IO settings. Its extremely fast, since ARMv7-M uses the UBFX instruction to get the individual flags from the 32-bit value.

This reduces the size of the virtual interface significantly. I would still keep the explicit interface for the static-type GPIO classes, since they cost nothing if not used, however also adding a configure method, since the compiler will likely optimize it all away for constant input values.

The Config_t values should ideally also be usable for other IO configs found on external IO expanders for example, that should then also be implemented using the virtual interface! This means there must be a minimal interface that may of course be value-optimized for the internal implementation, but still be useful for external implementations too.

There are some issues about naming things. Intuitively I'd call the interface modm::Gpio and the implementation modm::platform::Gpio, but it may lead to some ambiguity, when using namespace modm::platform; and using namespace modm;. Although the latter is not recommended. It's probably not a big issue in practice.

cc @chris-durand @rleh

@chris-durand
Copy link
Member

chris-durand commented May 15, 2021

👍 for doing more research on this.

I am not fully convinced yet a virtual interface is the right way to go. Do I understand correctly that it's purpose is to be able to use gpios on port expanders in a transparent way?

Generally value semantics and virtual don't go well together in C++. Look at the following code that would compile with this PR:

// slicing
modm::Gpio gpio = GpioA1();
gpio.set(); // crash, called pure-virtual function

This case could be prevented by making the assignment operator inaccessible but would not solve that users want to be able to do that.

Or trying to put different gpio types into a container. You either have the slicing problem or have to store pointers. That's why I came up with #543 initially. I would strongly prefer a value-semantics interface.

I am also unsure how an interface including port expanders could work in practice, especially with the concurrency model of resumable functions. Any operation on these gpio would need to be blocking and operations that fail can't report errors.

I would like to see how the virtual approach is superior for the case of mixed local/port expander gpios to using a value wrapper class containing a std::variant.

Config : uint32_t
{
/// @cond
_Mode = 0x8000'0000,
Copy link
Member

@chris-durand chris-durand May 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identifiers starting with _ + upper case letters are reserved in C++ for the standard library implementation.

@salkinium
Copy link
Member Author

salkinium commented May 15, 2021

I am not fully convinced yet a virtual interface is the right way to go.

I think if we have the overhead for runtime GPIO then we should accept 2-3 cycles overhead per virtual call. I need to check if the virtual tables get stored in Flash or RAM. I hope its Flash, otherwise it would be dumb…

Do I understand correctly that it's purpose is to be able to use gpios on port expanders in a transparent way?

Not just that. I've noticed particularly on STM32 that the differences between peripheral instances is just the *_Typedef pointer, but we're generating completely different types/functions for each, which leads to code duplication, particularly when using a lot of similiar peripherals in the same project.

The thing is that considering the overhead of the actual operation (ie. I2C transactions take forever, pushing a byte into UART isn't fast either), there's hardly any timing advantage for passing the Uart, Spi, I2c driver via template parameter, but excessive code bloat if you use the same external driver with different peripherals.
This is particularly true of GPIOs, where just changing the SPI chip select parameter generates a whole new class, which is not compensated with the speed gain of maybe a dozen cycles (the runtime GPIO API is surprisingly fast).

For GPIO I want to have both static and runtime APIs, since things like SoftwareGpioPort are much faster with templates.
But for the rest I want a runtime interface to reduce code bloat.

Generally value semantics and virtual don't go well together in C++.

Yeah, I already fell into this trap… 😒

I think there needs to be a bunch of predefined extern modm::platform::Gpio gpioA2; objects generated by lbuild that are garbage collected by the linker if not used. That way you don't have to worry about their lifetime.
Then you can do

extern modm::platform::Spi spi2;
extern modm::platform::Gpio gpioA2;
modm::IoExpander io_expander(/* modm::Spi& */ spi2, /* modm::Gpio& */ gpioA2);
modm::IoExpander::Gpio pin4(io_expander.pin(4));
modm::Driver driver(/* modm::Gpio& */  pin4);

Any operation on these gpio would need to be blocking and operations that fail can't report errors.

Yes, unfortunately that's how the adapters works today. It uses RF_CALL_BLOCKING() because it has to. We would either need to make the GPIO interface resumable (and pretty much all other interfaces resumable too), or we would use the Fibers interface. However, this code isn't making anything worse or better, it's just adding a virtual interface.

Not sure about error reporting, that would probably need to be done with modm_assert somehow. Otherwise every GPIO call would need to return Error or ResumableResult<Error>.

I would like to see how the virtual approach is superior for the case of mixed local/port expander gpios to using a value wrapper class containing a std::variant.

Hm, not an expert at std::variant but from what I read on Ze Interwebz™ you would still have to pass the types you want to use as a template parameter to the drivers? Something like

/*template<typename GpioTypes...>*/
Driver<modm::platform::Gpio, modm::IoExpander::Gpio>
driver(/*std::variant<GpioTypes...>*/ chip_select);

@chris-durand
Copy link
Member

chris-durand commented May 15, 2021

I played around with gcc to figure out if it optimizes out virtual calls in debug mode where it knows at compile time which method to call. This is the case if the dynamic type is statically known or it can prove the called method can't be overridden (final helps). With -Og it normally can't unless -ftree-dse ("tree dead store elimination") is passed. Don't know what this means for debugging but I expect it to be not that bad.

@chris-durand
Copy link
Member

chris-durand commented May 15, 2021

I think if we have the overhead for runtime GPIO then we should accept 2-3 cycles overhead per virtual call. I need to check if > > the virtual tables get stored in Flash or RAM. I hope its Flash, otherwise it would be dumb…

For time-critical tasks like bit-bang I would remain with a template-based interface. It could be a solution to allow the user to specify the gpio type as a template argument and have them pass an instance to the constructor. The type could be any runtime gpio. the static gpio class or any other gpio class from a port expander.

It could nevertheless be quite easy to use with class-type template argument deduction.

template<typename Scl, typename Sda>
class I2cBitbang;

modm::IoExpander::Gpio pin1 = ...;
modm::IoExpander::Gpio pin2 = ...;
I2cBitbang i2c{pin1, pin2}; // class template arguments deduced

I2cBitbang i2c2{gpioA1, gpioA2}; // class template arguments deduced

I2cBitbang<GpioA1, GpioA2> i2c3;  // constructor arguments defaulted to {}

EDIT: in case you have the same template arguments this would still prevent code bloat, but you have the possibility to choose

@chris-durand
Copy link
Member

Hm, not an expert at std::variant but from what I read on Ze Interwebz™ you would still have to pass the types you want to use as a template parameter to the drivers?

Yes, the use case was not entirely clear to me at that point. You would need to do that.

@rleh rleh added the stale ♾ label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants