In the STM32 port, it would be nice if the DMA configuration in the SPI device were only enabled during a DMA transaction, such that the SPI device could be used in non-DMA mode at other moments. Controlling the SPI_CR2_RXDMAEN and SPI_CR2_TXDMAEN flags in the SPI CR2 register would make this easy, I think. Thoughts?
As a general pattern, it would be nice if driver "config" structures could be temporary/local, and any values that need to exist longer term should be part of the driver structure itself. If these were specified in the config struct and simply copied to the driver struct during configuration, this would be fine. This means application code doesn't need to be responsible for maintaining additional static structs, which is nice, and some values in the config struct will typically be used only to configure some peripheral registers, so they would not need to exist statically for the life of the program, and save a few bytes possibly.
This one is the most insignificant, but the naming convention in the driver and config structs seems a little redundant. Having a prefix of spd or spc is not necessary, since those members are already scoped within the structure itself. It would be slightly easier to read spip->state than spip->spd_state everywhere, for example.
The callback supported driver in trunk is looking nice!
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
About the fields convention, it is used more or less consistently in the whole OS, this prefix thing is a bit old fashioned (think when ChibiOS/RT was really born, across 80s and 90s) and comes form an age where C compilers were far less sophisticated, I think it is because ancient compilers allowed to dereference a pointer using any field name, regardless the pointer type so it was common to add an unique prefix. It is not impossible that at some point it will change but it should be done in the whole system consistently. About the config structures, I see what you are suggesting but we should evaluate if the code required to fill a transient structure is larger that an equivalent const structure, it is possible that doing a "load immediate" + "store" for each field is larger than just the field value. stored in ROM. I'll make some tests about this.
Another consideration is that we would be trading ROM space for more valuable RAM space, think to all the data in the configuration structures that doest not really require to be stored in RAM, callback pointers as example, not all the configuration fields are register initializers.
About the new driver models, I am happy you noticed, what I am trying to do is to give most drivers both a synchronous and an asynchronous API. Imagine to read the ADC, change a PWM duty cycle from the ADC callback and also send the ADC buffer through SPI, everything from callback handler to callback handler, without threads involved and context switching. So far the drivers supporting the new model are ADC, PWM, SPI, I2C, UART. I am in doubt about CAN and MAC, those probably require interaction with threads in any possible scenario.
Giovanni
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
About the SPI, if I understood it well, you suggest to implement an IRQ-only fall-back mode in case the DMA is needed by another driver that shares, is this correctt?
Alternatively, it would be possible to dynamically allocate the shared DMAs between the various device drivers, the shared DMA could be "requested" when the driver xxxStart() function is invoked and "released" on the xxxStop(), this would be possible assuming that:
- The common DMA manager is modified to handle it and implement a DMA-IRQ redirector (the various drivers will have to share the ISR).
- The xxxStart() methods should be modified to handle the case that the requested DMA is already taken.
Giovanni
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
re: config structs - I see, I think you're right. As long as they're const and located in ROM, then it probably makes sense to keep them separate. Maybe a just recommendation in the docs to do this (declare them static const) would be enough.
With regard to the STM32 SPI driver, my particular use case was a little simpler - I didn't want to wait for a reschedule after an interrupt, and instead wanted to quickly write a byte or two via SPI synchronously. I haven't come across any cases in which I wanted to use the same DMA channels as you mentioned, so I think that part is OK the way it is.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I see, a function that exchanges few bytes synchronously only. Exchanges of few bytes happen a lot in the MMC driver too, probably bringing a "quick" version into the high level functions would be useful.
It looks like an excellent idea, the only potential problem I can see is the proliferation of functions in the SPI driver.
Giovanni
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I agree - such a large API becomes unwieldy. Even the similarity between the newer spixxxStart() and spixxx() routines makes it feel like they should somehow be parameterized instead of broken into separate routines, but I'm not sure what the answer is.
In the meantime, modifying the DMA routines to turn on DMA only during the transaction, instead of the entire time the SPI unit is enabled, would allow user code to implement it independently if they want.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Parameterizing would be possible but would also put a couple of IFs into the spiExchange() codepath (the checks for null parameters) and the application would have to always pass all four parameters. As an example spiIgnore(&SPID1, 4) would become spiExchange(&SPID1, 4, NULL, NULL), may be hidden in a macro but still 4 real parameters in code. I've considered this and the conclusion is that I am not sure if it is a good idea or not, efficiency or simplicity? I went for efficiency.
About the polled mode, i think it is possible even with the current implementation, this hypothetical spiQuickExchange() function would just have have to:
Specifically dedicated to single frame polled exchanges, done this way it should have minimal latency. The implementation should be polled because at high data rates the driver would waste more time serving an IRQ than just wait the operation completion (and the operation would be preemptable anyway). This issue is especially bad on chips without DMA channels.
The MMC_SPI driver would receive a switch to enable the polled mode for small transfers.
Giovanni
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
In my current (non-chibiOS STM32) project, I have created some polling routines like http://pastebin.com/cRszhcnK which is nice to avoid extra function calls in the case of multiple polling transfers, but it is probably overkill in this case.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
A little collection of thoughts:
In the STM32 port, it would be nice if the DMA configuration in the SPI device were only enabled during a DMA transaction, such that the SPI device could be used in non-DMA mode at other moments. Controlling the SPI_CR2_RXDMAEN and SPI_CR2_TXDMAEN flags in the SPI CR2 register would make this easy, I think. Thoughts?
As a general pattern, it would be nice if driver "config" structures could be temporary/local, and any values that need to exist longer term should be part of the driver structure itself. If these were specified in the config struct and simply copied to the driver struct during configuration, this would be fine. This means application code doesn't need to be responsible for maintaining additional static structs, which is nice, and some values in the config struct will typically be used only to configure some peripheral registers, so they would not need to exist statically for the life of the program, and save a few bytes possibly.
This one is the most insignificant, but the naming convention in the driver and config structs seems a little redundant. Having a prefix of spd or spc is not necessary, since those members are already scoped within the structure itself. It would be slightly easier to read spip->state than spip->spd_state everywhere, for example.
The callback supported driver in trunk is looking nice!
Interesting thoughs,
About the fields convention, it is used more or less consistently in the whole OS, this prefix thing is a bit old fashioned (think when ChibiOS/RT was really born, across 80s and 90s) and comes form an age where C compilers were far less sophisticated, I think it is because ancient compilers allowed to dereference a pointer using any field name, regardless the pointer type so it was common to add an unique prefix. It is not impossible that at some point it will change but it should be done in the whole system consistently. About the config structures, I see what you are suggesting but we should evaluate if the code required to fill a transient structure is larger that an equivalent const structure, it is possible that doing a "load immediate" + "store" for each field is larger than just the field value. stored in ROM. I'll make some tests about this.
Another consideration is that we would be trading ROM space for more valuable RAM space, think to all the data in the configuration structures that doest not really require to be stored in RAM, callback pointers as example, not all the configuration fields are register initializers.
About the new driver models, I am happy you noticed, what I am trying to do is to give most drivers both a synchronous and an asynchronous API. Imagine to read the ADC, change a PWM duty cycle from the ADC callback and also send the ADC buffer through SPI, everything from callback handler to callback handler, without threads involved and context switching. So far the drivers supporting the new model are ADC, PWM, SPI, I2C, UART. I am in doubt about CAN and MAC, those probably require interaction with threads in any possible scenario.
Giovanni
About the SPI, if I understood it well, you suggest to implement an IRQ-only fall-back mode in case the DMA is needed by another driver that shares, is this correctt?
Alternatively, it would be possible to dynamically allocate the shared DMAs between the various device drivers, the shared DMA could be "requested" when the driver xxxStart() function is invoked and "released" on the xxxStop(), this would be possible assuming that:
- The common DMA manager is modified to handle it and implement a DMA-IRQ redirector (the various drivers will have to share the ISR).
- The xxxStart() methods should be modified to handle the case that the requested DMA is already taken.
Giovanni
re: config structs - I see, I think you're right. As long as they're const and located in ROM, then it probably makes sense to keep them separate. Maybe a just recommendation in the docs to do this (declare them static const) would be enough.
With regard to the STM32 SPI driver, my particular use case was a little simpler - I didn't want to wait for a reschedule after an interrupt, and instead wanted to quickly write a byte or two via SPI synchronously. I haven't come across any cases in which I wanted to use the same DMA channels as you mentioned, so I think that part is OK the way it is.
I see, a function that exchanges few bytes synchronously only. Exchanges of few bytes happen a lot in the MMC driver too, probably bringing a "quick" version into the high level functions would be useful.
It looks like an excellent idea, the only potential problem I can see is the proliferation of functions in the SPI driver.
Giovanni
I agree - such a large API becomes unwieldy. Even the similarity between the newer spixxxStart() and spixxx() routines makes it feel like they should somehow be parameterized instead of broken into separate routines, but I'm not sure what the answer is.
In the meantime, modifying the DMA routines to turn on DMA only during the transaction, instead of the entire time the SPI unit is enabled, would allow user code to implement it independently if they want.
Parameterizing would be possible but would also put a couple of IFs into the spiExchange() codepath (the checks for null parameters) and the application would have to always pass all four parameters. As an example spiIgnore(&SPID1, 4) would become spiExchange(&SPID1, 4, NULL, NULL), may be hidden in a macro but still 4 real parameters in code. I've considered this and the conclusion is that I am not sure if it is a good idea or not, efficiency or simplicity? I went for efficiency.
About the polled mode, i think it is possible even with the current implementation, this hypothetical spiQuickExchange() function would just have have to:
Everything else should already be setup correctly.
Giovanni
What about a function done this way:
uint16_t spiPolledExchange(SPIDriver *spip, uint16_t frame);
Specifically dedicated to single frame polled exchanges, done this way it should have minimal latency. The implementation should be polled because at high data rates the driver would waste more time serving an IRQ than just wait the operation completion (and the operation would be preemptable anyway). This issue is especially bad on chips without DMA channels.
The MMC_SPI driver would receive a switch to enable the polled mode for small transfers.
Giovanni
That looks nice to me.
In my current (non-chibiOS STM32) project, I have created some polling routines like http://pastebin.com/cRszhcnK which is nice to avoid extra function calls in the case of multiple polling transfers, but it is probably overkill in this case.