DMA Enhancements « LeafLabs Garden

DMA Enhancements

(8 posts) (3 voices)

Started 1 year ago by robodude666
Latest reply from kernelcode

robodude666
Member

LeafLabs & Friends,

A few weeks ago I put in a pull request that enhanced the DMA Interrupt handler interface and removed unnecessary ISR clears. This pull request was denied & closed because it modifies an existing interface signature without a deprecation period -- which breaks a promise made as of 0.0.10's release.

I've spent some time making the changes mbolivar suggested in the pull request and commit comments, as well as rewriting the changes to allow for a deprecation period -- though I have not committed or pushed any changes to github yet. Throughout making the modifications I had trouble figuring out an elegant way of implementing them. I recently was on a week long camping trip, and while staring at a beautiful camp fire I had a realization: A wirish DMA class would be very handy. The proposed changes I made in my initial pull request would still be great to have, however they can be delayed until a later release to allow for a deprecation period where a non-elegant temporary "hack" is introduced to facilitate a transition.

Because I have little experience with writing publicly used APIs, I'd like to share what I have planned for feedback on better practices and use this thread for brainstorming.

Transitional Changes

Like mbolivar mentioned in the pull request comments, storing the latest dma_irq_cause within the handler config is probably the best option. This cause can then be accessed via a getter function, i.e. the existing dma_get_irq_cause. This will allow the current interface to remain the same and not break any existing applications that utilize dma interrupt handlers, while giving benefit to removing unnecessary dma_clear_isr_bit calls. The irq cause can be set within the handler dispatcher, similar to how my initial pull request handled it except that the config variable will be seat instead of passing it to the user's handler.

Changes Post-Deprecation Period

Once a deprecation period has passed, the dma_irq_cause can then be passed to the handler & the signatures can be modified. In addition, the dma_irq_cause from within the handler config can be removed as it is no longer needed (no deprecation period for this should be required as no one else but dma.c should be accessing that variable). dma_get_irq_cause can be reverted to its previous functionality of reading the isr bits & applying the masks, however it will no longer have the right of clearing the isr bits. This will be done separately within the dispatch handler.

More Elegant Solution

A more elegant solution would be to always set the interrupt enable bits for every channel configured. This will make the dispatcher trigger, allowing for the dma_irq_cause to be set every time - even if no user handler is setup. This would add "overhead" to uses where no handlers are used, however it would insure the correct irq cause is always available to the user. This method would use changes mentioned in the Transitional Changes but would not require the dma_get_irq_cause internal functionality to be reverted post-deprecation period, as the irq cause within the handler config would still be used.

The problem with this method is if the user modifies the CR2 bits manually, they may disable the interrupts which will break the irq_cause calculation for users not using the interrupt handlers.

Wirish DMA Class

Besides having getter/setter functionality for various aspects of the DMA controller settings, I feel the most important feature to have is the ability to very quickly reconfigure a particular DMA channel. This is handy because a particular channel can only serve to one peripheral at a time. I can see uses where a single channel can be reconfigured multiple times throughout the runtime of an application to communicate with multiple different peripherals on a single channel, for example communication with multiple different SPI devices, then being reconfigured to do UART transfer, etc. While different non-interfering channels should be used, you don't always get a choice making this a good feature to have.

The configuration should be handled via a struct, or a special configuration class, that contains the parameters you wish to use. An instance of this configuration struct/class can be made for each channel-peripheral combo you plan to use. Passing a reference to this struct/class to the DMA object will have it call the corresponding libmaple c functions and modify the DMA controller to these new settings. This will allow easy configuration between multiple peripherals/configurations on a single channel. Multiple levels of how complex the configuration should be can be used for improved performance. For example, only the # of transfers and memory addresses used, channel + full reconfiguration, etc. This will prevent every aspect of the DMA controller being changes when unnecessary.

As far as implementation details, the DMA class should represent one controller (DMAx, as per RM0008 notation) with DMA1 and DMA2 pre-defined for the corresponding maple boards. The class, along with every Wirish class that represents a hardware peripheral, should be a singleton to prevent multiple instances and register changes between instances (shouldn't be a problem with good programming practices, but safety built-in rarely hurts).

Some things I have questions about best practices for:

- Should the user allocate the RAM & provide the DMA class a pointer to it, or allow the DMA class to manage the memory?

- If the DMA class should manage the memory, how should the user get access to it after peripheral to RAM transfer is completed? Give the user the raw pointer, or provide an iterator #define macro? (faster than a function, and "safer" than giving a raw pointer... ?)

That's about it for now. I'd love to hear what ya'll have to think. I really love my Maple and feel the DMA controller is a very important hardware feature and should be taken advantage of with an easy-to-use interface like HardwareSPI and HardwareSerial makes SPI and Serial simple!

I'd love to start work on this, unless there' feedback this/these are bad ideas.

-robodude666

Posted 1 year ago #
mbolivar
LeafLabs
I just noticed this post while googling around for DMA-related threads. Interesting stuff, sorry I'm so late to the party!

Changes to libmaple proper:

I mostly like the idea of feeding in the IRQ cause by default, but I do worry a little bit about performance-critical uses where computing that information might be a waste of time. For instance, consider a situation where only the error interrupt is enabled, and it's important to react very quickly (e.g. to shut down another peripheral) in case of error.

The obvious thing to do is add extra bits to dma_dev that determine whether this information is computed, on a channel-by-channel basis. This still introduces a little extra overhead, but it should be pretty minimal.

Wirish DMA class:

When adding to Wirish, our general rule is to do things in the Arduino style, unless it's odious. For this reason, I think passing a struct to configure the DMA peripheral might not be the right thing. The "usual Arduino style" is to provide a method for each knob you might want to twist when configuring a peripheral, which (playing devil's advocate a bit here) doesn't seem too unreasonable, and would likely aid the exposition when writing up the documentation.

Here's a candidate interface in this style, comments welcome:
```
class DMA {
public:
    // Constructor, takes number of DMA controller
    DMA(uint8 dmaNumber);

    // Sets the channel, but doesn't start the transfer
    void begin(dma_channel channel);

    // Address and size config
    void setPeripheralAddress(void *paddr);
    void setPeripheralSize(dma_xfer_size psize);
    void setMemoryAddress(void *maddr);
    void setMemorySize(dma_xfer_size msize);

    // Priority config
    void setPriority(dma_priority p);

    // dma_mode_flags twiddlers
    void memoryToMemoryMode();
    void circularMode();
    void fromMemoryMode();
    // ... etc.

    // Interrupt handler config
    //
    // Calling one of these causes the appropriate DMA_TRNS_ERR,
    // DMA_HALF_TRNS, DMA_TRNS_CMPLT flag to get set in the eventual
    // call to dma_setup_transfer().
    //
    // If any of these has been called, DMA::start() will pass
    // dma_attach_interrupt() a handler which uses dma_get_irq_cause()
    // to call the appropriate user foo_handler().  (Users that don't
    // like this overhead can use dma_attach_interrupt() directly).
    void onError(void (*err_handler)(void));
    void onCompletion(void (*cmplt_handler)(void));
    void onHalfCompletion(void (*half_handler)(void));

    // Begin/end transfers
    void start();
    void stop();

    // Manually check if a transfer is done
    void isComplete();
};
```
Instances are constructed a la HardwareSPI; e.g. DMA dma1(1);. The usage would be similar to Wire -- call dma1.begin(some_channel), do your config, call dma1.start(), voila.

You don't lose reconfiguration here: call stop(), maybe call begin(), then call the appropriate knob twisting methods with whatever's different, call start() again.

Regarding singletons, I agree that that's the Right Thing, but I worry that they don't really fit in with the simplified programming model we're after here. E.g. introducing factory methods seems like too much extra cognitive load for the safety benefits. I know that's icky from an engineering perspective, but lots of things about Wirish are icky in that regard; it's just part of the cost of doing business when you're optimizing for simplicity. See e.g. the HardwareSPI class.

Re: best practices: Let the user allocate the memory. That way, they can pick the right buffer size for them, and they can do their own error handling however they see fit. This is also consistent with much of the rest of libmaple (e.g. HardwareSPI::read(uint8*,uint32), usbReceiveBytes(), i2c_master_xfer()).
Posted 1 year ago #
mbolivar
LeafLabs
Other random thoughts on making things easier:

It's gotta be worthwhile to add extra methods to cover the common cases. For instance, I imagine that getting bytes into and out of SPI/USART will be pretty universally desired, and likewise for ADC readings (there's a parallel thread on DMAing dual ADC readings, for instance).

Something like this?
```
DMA dma(1);

#define NREADINGS 10
uint16 adc_readings[NREADINGS];

void setup() {
    dma.beginADCLoop(adc_readings, NREADINGS);
    dma.onCompletion(finishedConverting);
    dma.start();
}

void finishedConverting() {
    // do something with this round of converted values
}
```
Where DMA::beginADCLoop() would use &ADC1_BASE->DR for the peripheral address, the user's buffer for the memory address, and configure the channel to fill up the buffer repeatedly (using DMA_MINC_MODE | DMA_CIRC_MODE).
Posted 1 year ago #
robodude666
Member

mbolivar,

Welcome to the party! I haven't worked on this much since I posted about it, so sorry for my late reply, in case people thought it'd be a bad idea, but it's been something I've wanted to do (and could really use) so I'll be picking it up soon.

So here we go:

Would a ~100 nanosecond delay for turning off an ADC, SPI, USART, etc. really make a difference? I'd understand for people who have a task being completed very frequently that a ~100ns delay would cause very large delays cumulatively. I don't expect to receive a DMA error often, unless you're doing it wrong.

I personally really hate Wirish paradigms. They're so silly!

I got the idea of a struct/factory from the way STM has their official library. They use a struct to hold all configuration options that is passed along. It ends up being very useful and requires almost no documentation to understand what's going on.

If you have, for example, a few SPI devices connected to SPI1 and you're using the DMA controller to communicate to it, but each device has its own set of configurations (memory address, # bytes, interrupt options, priority, etc.) you would need to set all of those over and over when repurposing the controller. Why not simply have all those settings stored in a struct that you can pass along to reconfigure the controller? Sure, you'd still have all the other wirish methods for all the knobs you want to twist, but when having to change multiple options at once you'll have an easier time doing it.

However... I suppose, if the DMA class won't be a singleton then any number of DMA objects can exist. You can have a DMA object for each one of your SPI devices attached, and simply begin/end/start/whateverYouWantToCallIt() when you're ready. The DMA class can update the DMA registers every time a transfer starts to make sure its settings are being used. The wirish methods for knob twisting will simply set local member variables that won't touch the registers until the DMA transfer is actually requested. I'd be okay with such an approach. Singletons are a bit confusing for an embedded platform, I suppose.

The problem I was having about letting the user allocate the memory was that: what if they allocate the wrong amount? Or even worse, too little and the DMA controller starts writing into space that's not part of the allocated block? Also, would you then depend on the programmer being good to prevent any dangling pointers? (They free the allocated memory but forget to first NULL out the pointer passed to the DMA class, eep!)

I think things that are going to be common practices: reading ADC, SPI, USART, writing to ADC, SPI, USART, etc. should get their own subclasses (at least on a peripheral level) with these connivence methods instead of clogging up the main DMA interface with methods that may not apply to that particular object.

-robodude666

Posted 1 year ago #
mbolivar
LeafLabs
Would a ~100 nanosecond delay for turning off an ADC, SPI, USART, etc. really make a difference? I'd understand for people who have a task being completed very frequently that a ~100ns delay would cause very large delays cumulatively. I don't expect to receive a DMA error often, unless you're doing it wrong.

The error case was just a random example that I can't really back up with use cases, so I'll drop it.

What I do know is that we've gotten a fair amount of feedback about IRQ handlers being slow, for instance, timer CC interrupt overhead has bitten people in the past. Because of that, I'm wary of introducing mandatory overhead into IRQ handlers.

I may well be being needlessly conservative. If you want to do it this way, go ahead; we'll see how it plays out in practice and proceed from there.

As another random devil's-advocate type suggestion, how about the Wirish DMA class automatically passing the dma_irq_cause to the user's attached handlers? That way, using libmaple proper wouldn't incur extra ovehead unless it's wanted, but using Wirish (as usual) gets you more convenience at the cost of some performance.

I got the idea of a struct/factory from the way STM has their official library. They use a struct to hold all configuration options that is passed along. It ends up being very useful and requires almost no documentation to understand what's going on.

We've read some of the ST code as well, and honestly, we don't like it.

Using ST's struct-style libs requires an intimate knowledge of the register map in order to get anything done, so saying it "requires almost no documentation to understand what's going on" is only true if you don't count RM0008 as documentation. libmaple might be close to the metal, but it's not _that_ close; if we don't provide some nicer abstractions, we might as well just use the ST libs.

If you have, for example, a few SPI devices connected to SPI1 and you're using the DMA controller to communicate to it, but each device has its own set of configurations (memory address, # bytes, interrupt options, priority, etc.) you would need to set all of those over and over when repurposing the controller. Why not simply have all those settings stored in a struct that you can pass along to reconfigure the controller? Sure, you'd still have all the other wirish methods for all the knobs you want to twist, but when having to change multiple options at once you'll have an easier time doing it.

This is definitely desirable, but see above for why we avoid the ST style.

And I don't think that having knob-twisting methods precludes initializing your peripherals with mostly-default values. libmaple proper uses xxx_foreach() routines to get this done in C, e.g. setupTimers() in boards.cpp:

https://github.com/leaflabs/libmaple/blob/0.0.11/wirish/boards.cpp#L115

In C++, you can use default parameter values to get things done. So e.g. you might have begin() call out to the knob-twisters:
```
void DMAClass::begin(uint32 enabled_interrupts=(DMA_TRNS_ERR | DMA_TRNS_CMPLT),
                      dma_priority priority=DMA_PRIORITY_LOW,
                      ...)
{
    this->enableInterrupts(enabled_interrupts);
    this->setPriority(priority);
    // .. etc.
}
```
Users with different preferences could wrap DMAClass::begin() with a helper function of their own that reset the defaults they didn't like.

It also comes down to a question of consistency. For better or worse, the Wirish interfaces are modeled after Arduino's, with some of the bad parts removed. Changing that makes me nervous; as time passes, we could get more and more different styles creeping in to Wirish, with the result being a library that's hard for beginners to make a mental model of.

What do you think about that?

The problem I was having about letting the user allocate the memory was that: what if they allocate the wrong amount? Or even worse, too little and the DMA controller starts writing into space that's not part of the allocated block? Also, would you then depend on the programmer being good to prevent any dangling pointers? (They free the allocated memory but forget to first NULL out the pointer passed to the DMA class, eep!)

If they allocate the wrong amount, they've got a bug. C and C++ don't do bounds checking; c'est la vie.

As far as dangling pointers go, following Wiring, we encourage a programming model where buffers are fixed-size and allocated statically. This prevents the usual memory management issues.

If someone wants to manage their memory dynamically, then, as usual in C and C++, it's up to them to do it correctly.

I think things that are going to be common practices: reading ADC, SPI, USART, writing to ADC, SPI, USART, etc. should get their own subclasses (at least on a peripheral level) with these connivence methods instead of clogging up the main DMA interface with methods that may not apply to that particular object.

That seems like a good idea. It'll also help partition up the documentation so people can laser in on the parts they care about.
Posted 1 year ago #
robodude666
Member

I personally really hate Wirish paradigms. They're so silly!

Sorry. I meant to say Arduino's and Wiring's paradigms are silly; not LeafLab's Wirish. I know that LeafLab's Wirish only follow those paradigms.

Now.. Putting the irq cause into the wirish handlers while keeping the libmaple ones as lean as possible would probably be the best approach to it.

That is also true. While ST's library does makes some sense, I did find myself referring to the RM0008 documentation to understand what EXACTLY was going on with the registers. An average arduino user would go insane from using ST's library, not that the average arduino user would be using structs, singletons, factory methods, etc. in the first place... So, point taken: KISS.

Well, you've given me a lot of things to think about, and settled many of my concerns. I've started working on another project at the moment (gasp! I'm actually finishing this one.) but will start work on this class very soon.

Posted 1 year ago #
mbolivar
LeafLabs

Cool! Sounds good; looking forward to using your code.

Posted 1 year ago #

kernelcode
Member

Hi, I'm finding it tricky to jump into all of this development talk, having never used an Arduino and having got a Maple for a University project, but hopefully you can bear with me.

Anyhow, my project involves grabbing samples from an ADC, doing some crunching and spitting them out on [some serial interface]. The requirements are such that I will almost certainly need to use DMA (and I'd like to anyway - it's so neat!) so I've started implementing some C++ wrappers for the dma.h libmaple functions, and have also implemented a (scruffy) DMA USART driver based off that. Hopefully this week I will get a DMA ADC driver going too.

So my question is, how much is already implemented? I don't want to re-invent the wheel, and conversely I would really like to contribute my work back to the community (pending approval from my supervisor) if it would be useful.

For an idea of where I'm at, here is my DMA class declaration:

#include "dma.h"

class dma_object {

    private:
        dma_object( void );             ///< Device and Channel are req'd
        dma_dev *_dev;                  ///< DMA Device (DMA1 or DMA2)
        dma_channel _channel;           ///< DMA Channel to use
        uint32 _mode;                   ///< DMA Mode, ORed dma_mode_flags
        __io void * _m_address;         ///< Address in memory
        __io void * _p_address;         ///< Address in peripheral
        uint16 _transfers;              ///< Number of transfers
        dma_xfer_size _p_width;         ///< Data width (peripheral)
        dma_xfer_size _m_width;         ///< Data width (memory)
        dma_priority _priority;         ///< Priority (defaults to DMA_PRIORITY_LOW)
        void (*_handler)();             ///< irq handler
        void setup_xfer();              ///< Calls dma_set_... and dma_attach_interrupt

    public:
        /// Initialises DMA channel, sets some defaults
        dma_object(dma_dev * dev, dma_channel channel); 

        /// Full configuration information
        void configure( __io void * m_address, __io void * p_address,
                        dma_xfer_size m_width, dma_xfer_size p_width,
                        uint32 mode, uint16 transfers);

        inline void setMode     ( uint32 mode)
                                { _mode = mode; };
        inline void setMAddr    ( __io void* m_address )
                                { _m_address = m_address; };
        inline void setPAddr    ( __io void* p_address )
                                { _p_address = p_address; };
        inline void setTransfers( uint16 transfers )
                                { _transfers = transfers; };
        inline void setWidths   ( dma_xfer_size m_width,
                                  dma_xfer_size p_width )
                                { _m_width = m_width; _p_width = p_width; };
        inline void setPriority ( dma_priority priority)
                                { _priority = priority; };

        void enable ( void );
        void disable( void );

        void attachInterrupt( void (*handler)(void));
        void detachInterrupt( void );

};

This is in no way complete, or 'Wirish', and doesn't fit the coding conventions (yet) but hopefully someone can give me some input and maybe I can help out :-)

DMA Enhancements

Reply