changed mcp2515 driver to use nonblocking spi

[kikass13]

• [x] change mcp2515 to use proper modm::can driver api
• [x] change mcp2515 to use the nonblocking spi api
  • mostly, initialization routines are still blocking but i guess tahts fine
• [x] add example
• [x] utilize attachConfigurationHandler for switching device spi types
• [x] test example on a nucleo ~or discovery board~
• optional:
  • [x] utilize attachConfigurationHandler for switching device clock speeds

[rleh]

Another addition: This driver thould use the SpiDevice abstraction and should make use of void attachConfigurationHandler(Spi::ConfigurationHandler handler)! @kikass13: We need this, because we have multiple MCP2515 and one ADIS16470 connected to the same SPI.

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

[chris-durand]

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

That requires to reconfigure SPI timings when the SpiDevice acquires the SpiMaster. A method similar to the configuration handler could be used but this time it is provided externally by the user and not the driver. The handler function pointer could be put into the SpiDevice class. It would be called inside of the SpiMaster implementation. If not used the runtime overhead of the solution would be a not taken branch with a nullptr check inside of the SpiMaster::acquire() method.

[chris-durand]

@rleh I have quickly hacked together a sub-optimal implementation for switching the SPI baudrate with multiple devices. This is just a rough proof of concept for STM32 only and makes changes to the SpiMaster API.

https://github.com/chris-durand/modm/tree/wip_spi_user_conf_handler

[kikass13]

Another addition: This driver thould use the SpiDevice abstraction and should make use of void attachConfigurationHandler(Spi::ConfigurationHandler handler)! @kikass13: We need this, because we have multiple MCP2515 and one ADIS16470 connected to the same SPI.

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

[chris-durand]

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

The configuration handler is actually quite simple. If you have multiple devices on one bus that require different SPI modes you'll need to reconfigure the mode whenever you run a transfer on the next device. You can define a callback that configures the required parameters and modm will call it automatically when needed. Here is an example.

EDIT: if you mean the SpiDevice with your comment, it's a class that manages shared access to one SpiMaster by many device drivers. You basically inherit your driver from SpiDevice<SpiMaster> and put RF_WAIT_UNTIL(this->acquireMaster()); before you do anything with the SPI and call releaseMaster() when you are done. That way it's made sure only one driver uses the bus at the same time. You can look at basically any other SPI based driver to see how it is used.

[kikass13]

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

The configuration handler is actually quite simple. If you have multiple devices on one bus that require different SPI modes you'll need to reconfigure the mode whenever you run a transfer on the next device. You can define a callback that configures the required parameters and modm will call it automatically when needed. Here is an example.

EDIT: if you mean the SpiDevice with your comment, it's a class that manages shared access to one SpiMaster by many device drivers. You basically inherit your driver from SpiDevice<SpiMaster> and put RF_WAIT_UNTIL(this->acquireMaster()); before you do anything with the SPI and call releaseMaster() when you are done. That way it's made sure only one driver uses the bus at the same time. You can look at basically any other SPI based driver to see how it is used.

Yeah the second one .. you explained what it does quite well, thanks :)

[kikass13]

@rleh I have quickly hacked together a sub-optimal implementation for switching the SPI baudrate with multiple devices. This is just a rough proof of concept for STM32 only and makes changes to the SpiMaster API.

https://github.com/chris-durand/modm/tree/wip_spi_user_conf_handler

@rleh @chris-durand i tested this and seems to work (one would have to check with an oscilloscope if it actually happened, and I don't have one at home :P)

[kikass13]

SpiDevice test seems to be broken? Or did I do something wrong?

• https://github.com/modm-io/modm/runs/5065030473?check_suite_focus=true

[chris-durand]

SpiDevice test seems to be broken? Or did I do something wrong?

* https://github.com/modm-io/modm/runs/5065030473?check_suite_focus=true

That is because of my experimental commit that was cherry-picked. It is only a proof of concept for STM32. Remove it and everything will be fine :)

[rleh]

I think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options, everything else doesn't work consistently and leads to strange behavior, especially when multiple SPI drivers access SPI in different orders one after the other.

The "relevant" SPI options:

• Frequency
• DataMode
• DataOrder
• ...?

To enforce this I would suggest to implement these three values (with usable default values) in addition to the configurationHandler in the SpiDevice. The SpiDevice class is able to configure this values very efficiently by a single call to the (protected) initialize() function of the respective SpiMaster, whereas calls like SpiMaster::setDataMode(Mode3); SpiMaster::setDataOrder(MsbFirst); in the configurationHandler might reinitialize the SPI peripheral multiple times.

The configurationHandler is optional and should only be used to configure additional IOs/multiplexers/... .

What do you think @chris-durand?

[kikass13]

I think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options, everything else doesn't work consistently and leads to strange behavior, especially when multiple SPI drivers access SPI in different orders one after the other. ...

Well I dont see why the SpiDevice is currently not a default base class with mandatory (pure virtual) overrides To me the driver api looks inconsistent in multiple places

• everything is static (or somethimes not when utilizing protothreads, as they require a non static class structure),
• unclear positions of setable /mostly important driver specific options / clocks )
• the ability for a driver to access the bus without a mutex (SpiDevice class)
• to name a few things

[chris-durand]

I think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options, everything else doesn't work consistently and leads to strange behavior, especially when multiple SPI drivers access SPI in different orders one after the other.

I agree that it makes sense to make it mandatory for the SPI modes. I am not sure how it would work for the frequency. The frequency is a compile time template parameter in the SPI master because we want to verify it at compile-time. We would need a second API that allows runtime values without compile-time checking to make it configurable.

Furthermore the clock configuration could require some other platform specific settings on future platforms. I would prefer to decouple it from the device drivers. I would rather let the user provide a call back for the speed configuration that is called when the SPI device is acquired.

[chris-durand]

Well I dont see why the SpiDevice is currently not a default base class with mandatory (pure virtual) overrides.

I think there is no need for runtime polymorphism and dynamic dispatch for that use case. Couldn't we just pass a configuration as a non-type template parameter to the SpiDevice for setting the modes? SPI modes are known statically at compile time.

It's not a real is-a relationship for let's say an MCP2515 and a generic SPI device. I would argue a MCP2515 is not a specific version of an SPI interface to be used as such. SPI is an implementation detail, in its public interface it's a CAN interface.

[chris-durand]

To enforce this I would suggest to implement these three values (with usable default values) in addition to the configurationHandler in the SpiDevice. The SpiDevice class is able to configure this values very efficiently by a single call to the (protected) initialize() function of the respective SpiMaster, whereas calls like SpiMaster::setDataMode(Mode3); SpiMaster::setDataOrder(MsbFirst); in the configurationHandler might reinitialize the SPI peripheral multiple times.

Looking at the STM32 SPI driver it is actually the opposite. initialize() disables the device, configures all settings, enables master mode and enables the device again. Changing the data mode can be performed by one single write when no SPI transaction is running. initialize() is less efficient. I am not sure if it is different on other platforms. In that case introducing a new function to set these parameters in one go could make sense.

I would change the SpiDevice to take a struct value as a template argument that contains relevant settings (mode, data order, any more?). The drivers will be forced to define them and drivers would not touch the configuration handler anymore which is left entirely to the users. The configuration handler could also be used to configure the frequency if the use-case requires it.

[kikass13]

I think there is no need for runtime polymorphism and dynamic dispatch for that use case.

static polymorph is a thing :/

[chris-durand]

static polymorph is a thing :/

Yes, but it is normally not done using virtual functions. A classic C++ pattern for static polymorphism is for example CRTP. De-virtualization optimizations usually don't work well at -Os and -Og so I would rather not want to rely on virtual functions where no dynamic polymorphism is required.

Which virtual functions are you thinking of when you said SpiDevice should have them?

[kikass13]

static polymorph is a thing :/

Yes, but it is normally not done using virtual functions. A classic C++ pattern for static polymorphism is for example CRTP. De-virtualization optimizations usually don't work well at -Os and -Og so I would rather not want to rely on virtual functions where no dynamic polymorphism is required.

Which virtual functions are you thinking of when you said SpiDevice should have them?

either pure virtual or a crtp adapter, where release and acquire master is done in every spi command automatically. you mutex bus access (which is a wise thing to do) . The fact that a driver can decide to NOT do that is really weird to me :D

[chris-durand]

either pure virtual or a crtp adapter, where release and acquire master is done in every spi command automatically. you mutex bus access (which is a wise thing to do) . The fact that a driver can decide to NOT do that is really weird to me :D

Ah, now I understand what you would like to do. Enforcing that the lock on the SPI is held while it is used sounds like a good idea. If I understand correctly, you'd like to acquire the lock automatically at the start of the SPI transfer and release it afterwards? In many drivers one transaction consists of multiple subsequent transfers that can't be split up. How would you deal with that with that concept?

[salkinium]

think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options

When I added the config handler, it was intended to be set by the application and NOT the driver, to avoid exactly this issue. The developer knows what other devices are on the same bus and thus knows what settings to change. There is also the issue that some devices can only operate at lower speeds (mostly I2C related) and will electrically malfunction (something with slew rates and capacity? I'm not an EE…) on higher speeds. In that case the driver cannot know this and thus setting a higher speed automatically is bad.

If you only have one device on the bus, you don't need the config handler at all, it is really supposed to be only a user config handler, not a driver config handler. Obviously this needs to be documented better…

[chris-durand]

In that case the driver cannot know this and thus setting a higher speed automatically is bad.

I 100% agree, but for the SPI mode it makes sense that the driver configures it. That is not a user setting and a property of the device. The driver should somehow configure it and it should work with a shared bus as well. Currently this is not enforced in modm and won't work with most drivers if they require different modes. @salkinium How would you solve that?

[salkinium]

How would you solve that?

I would have a static function that deals only with the configuration (perhaps with additional arguments like speed). It could be called outside the driver when only that device is on the bus, or inside the config handler that is set by the application.

template< class SpiMaster >
class SpiDevice
{
	static void
	configureBus()
	{
		SpiMaster::setMode(SpiMode::Mode3);
	}
};
SpiDevice<SpiMaster> device;

SpiMaster::initialize<SystemClock, speed>();
// configure once if alone on the bus
device.configureBus();

// Configure if multiple devices on the bus.
device.setConfigHandler([](){ device.configureBus(); });

[kikass13]

either pure virtual or a crtp adapter, where release and acquire master is done in every spi command automatically. you mutex bus access (which is a wise thing to do) . The fact that a driver can decide to NOT do that is really weird to me :D

Ah, now I understand what you would like to do. Enforcing that the lock on the SPI is held while it is used sounds like a good idea. If I understand correctly, you'd like to acquire the lock automatically at the start of the SPI transfer and release it afterwards? In many drivers one transaction consists of multiple subsequent transfers that can't be split up. How would you deal with that with that concept?

The modern way would be to write a exec function and provide a lambda.

The lambda will be executed in a proper context (acquire mutex, set bus options, set bus signals low/high depending on the protocol) and execute user code afterwards.

The user should imo only be able to define actions, whulle the driver should enforce context

[chris-durand]

The modern way would be to write a exec function and provide a lambda.

Yes, but that does not work with resumable function macros. The resumable function is a gigantic switch case statement. If you put a case of a big switch defined outside the lambda inside of the lambda the compiler screams at you. The resumable functions have to be replaced, that is the condition for many nice things :).

[kikass13]

The modern way would be to write a exec function and provide a lambda.

Yes, but that does not work with resumable function macros. The resumable function is a gigantic switch case statement. If you put a case of a big switch defined outside the lambda inside of the lambda the compiler screams at you. The resumable functions have to be replaced, that is the condition for many nice things :).

Yeah, for that you would probably need a proper executor pattern (something to give tasks to, that can execute these tasks in proper scheduled manner (aka protothreads or whatever ). Not impossible , bus would be was more complicated then necessary I guess

Basically, when dealing with protothreads, nothing should be executed from the user context, sounds bad for this purpose I guess

[chris-durand]

I would have a static function that deals only with the configuration (perhaps with additional arguments like speed). It could be called outside the driver when only that device is on the bus, or inside the config handler that is set by the application.

@salkinium This SpiDevice in your code example, is this modm::SpiDevice<SpiMaster>? If yes, the device specific mode config options have to be passed into that from the driver. For that I have suggested passing the mode config as a non-type template parameter to modm::SpiDevice. Your example adapted:

template< class SpiMaster, SpiConfig spiConfig >
class modm::SpiDevice
{
	static void
	configureBus()
	{
		SpiMaster::setMode(spiConfig.mode);
                // maybe option for data order or others as well, don't know ...
	}
};

constexpr SpiConfig someDriverConfig{...};

// then as usual inherit from SpiDevice and specify the config
template< class SpiMaster >
class SomeDriver : public SpiDevice<SpiMaster, someDriverConfig>
{…};

Then defining the modes is mandatory. Many modm drivers using Mode0 can't be used in a shared bus configuration because they assume the default mode 0.

[salkinium]

No, I meant this to be the driver class, but I just forgot about the SpiDevice class, cos I haven't worked with it for so long… It should just be a separate function with common name pattern (configureBus) that can optionally extended with or split-up into more specific configs (configureBusSpecificThing), but they are just functions that are assembled by the application. We could also use modm::inplace_function for the config handler to use the lambda properly (modulo race conditions with the interrupt).

[salkinium]

I specifically do not want configs that are automatically called by default, since that does magic on the bus and can be horrible to debug. Everything should be explicitly done by the application, but in a comfortable way, ie. the most common config use-case is given simply by the driver.configureBus() function.

[chris-durand]

It should just be a separate function with common name pattern (configureBus) that can optionally extended with or split-up into more specific configs (configureBusSpecificThing)

I specifically do not want configs that are automatically called by default, since that does magic on the bus and can be horrible to debug. Everything should be explicitly done by the application, but in a comfortable way, ie. the most common config use-case is given simply by the driver.configureBus() function.

I do not object that you want something explicit, but I dislike the fact that this is just a convention and not a compiler-enforced API. Currently it's a mess: some drivers use the configuration handler from within the driver, some don't configure the mode and assume it's Mode0, etc. I would like a solution where the compiler yells at the driver author if setting the SPI mode is not addressed. And application code should not have to mess with what the SPI mode for a specific device is.

In probably 99% of the cases a configureBus() function would do nothing more than SpiMaster::setMode(SpiMode::ModeX); To cover that simple case modm::SpiDevice could implement a default configureBus(). The driver inherits for example from modm::SpiDevice<SpiMaster, SpiMode::Mode3>. Now we can have a configureBus() by default that does the right thing in almost all cases and the driver is ready for shared bus operation. If it compiles it works, no need to look for this in reviews and no implicit magic.

[rleh]

Then defining the modes is mandatory. Many modm drivers using Mode0 can't be used in a shared bus configuration because they assume the default mode 0.

Currently almost all spi devices in modm assume Mode0 and MsbFirst, never configure it and fail if they share a bus with a device that uses other mode or data order. Configuring mode and data order is fast (no need to stop and restart the device (on STM32)), so I think this should in any case happen in acquireMaster(). These properties definitely belong in the device drivers. When I started the discussion earlier, I imagined roughly what @chris-durand suggested in https://github.com/modm-io/modm/pull/817#issuecomment-1030425892.

Our SpiMaster implementations have (currently) no more configurable properties than the mentioned three: DataMode, DataOrder and frequency. (+ STM32 non-UART SPI can configure DataLength.)

I don't see why the SPI frequency should not be handled by the device drivers, since every spi peripheral datasheet clearly states a maximum SPI frequency (and sometimes also a minimal frequency). The STM32, it's clock configuration and the PCB also limit the frequency to some value.

Idea:

• We consider the frequency SpiMaster currently get initialized with as the STM32/board maximum frequency. (Could possibly be defaulted to the max frequency possible.)
• The SpiConfig struct (mandatory template parameter of every modm::SpiDevice) gets minFreq and maxFreq entries. modm::SpiDevice::acquireMaster() applies DataOrder and DataMode settings and checks if the current frequency is within [minFreq, maxFreq], only if that is not the case the SpiMaster frequency is changed to maxFreq (with the known compiletime checks).

This way the user doesn't have to look up the allowed frequencies for each SPI peripheral in the datasheet and in the long run we could even make the (maximum) frequency parameter optional for the SpiMaster::initialize().

If a configurable property is added in the future, it will be added as an entry with the expected behavior in SpiConfig, and modm::SpiDevice will take care of configuring it. Otherwise, every driver, or worse, every application code, will have to be adjusted when such changes are made.

Maybe it also makes sense at SpiMaster to deprecate setDataOrder() and setDataMode() and replace them with a single setSpiConfig<SpiConfig>() (or similar). Then we can generate nice error messages at compiletime if there are properties of SPI-Master to be configured that don't exist.

The (existing) configuration handler then remains for the user to configure everything that goes beyond and is not misused by the device drivers.

[salkinium]

Yeah, I'm fine with that too. It's more thought out with the config struct.