dma_channel_transfer_[from/to]_buffer_now() functions have inconsistent interface #158

New issue

Closed

opened 2021-02-18 16:00:34 -06:00 by geurtv · 6 comments

geurtv commented

2021-02-18 16:00:34 -06:00

(Migrated from github.com)

Analogous to dma_channel_set_read_addr(), dma_channel_set_write_addr() and dma_channel_configure():

The read_addr parameter of dma_channel_transfer_from_buffer_now() is now void* but should be const volatile void*.
The write_addr parameter of dma_channel_transfer_to_buffer_now() is now void* but should be volatile void*.

Analogous to `dma_channel_set_read_addr()`, `dma_channel_set_write_addr()` and `dma_channel_configure()`: The `read_addr` parameter of `dma_channel_transfer_from_buffer_now()` is now `void*` but should be `const volatile void*`. The `write_addr` parameter of `dma_channel_transfer_to_buffer_now()` is now `void*` but should be `volatile void*`.

Reneg973 commented

2021-05-09 12:00:18 -05:00

(Migrated from github.com)

I would do it vice versa, means removing volatile from the first 3 functions. Cause, it is superfluous.

geurtv commented

2021-05-22 04:53:11 -05:00

(Migrated from github.com)

Fair enough. To be honest I didn't actually care about the volatile part. It's the missing const for the read_addr parameter of dma_channel_transfer_from_buffer_now that makes code fail to compile. You are now likely required to add a const cast.

Fair enough. To be honest I didn't actually care about the `volatile` part. It's the missing `const` for the `read_addr` parameter of `dma_channel_transfer_from_buffer_now` that makes code fail to compile. You are now likely required to add a `const cast`.

kilograham commented

2021-05-22 06:57:59 -05:00

(Migrated from github.com)

If you open a PR for which the entire pico_sdk (i.e. including kitchen_sink) compiles, then I'll take a look

geurtv commented

2021-05-26 03:38:24 -05:00

(Migrated from github.com)

On second thought, adding volatile isn't completely useless. Due to compiler optimizations the following code will always return 42 when the byte variable isn't volatile and without a volatile void* write_addr it won't compile.

uint8_t f(unsigned ch)
{
    volatile uint8_t byte = 42;
    dma_channel_transfer_to_buffer_now(ch, &byte, 1);
    while (dma_channel_is_busy(ch)) ;
    return byte;
}

Although probably the proper way to handle this is to remove the volatile on byte and instead add some memory fence before return. So either C11/C++11's atomic_signal_fence() or (the undocumented) __compiler_memory_barrier().

On second thought, adding `volatile` isn't completely useless. Due to compiler optimizations the following code will always return `42` when the `byte` variable isn't volatile and without a `volatile void* write_addr` it won't compile. ```c uint8_t f(unsigned ch) { volatile uint8_t byte = 42; dma_channel_transfer_to_buffer_now(ch, &byte, 1); while (dma_channel_is_busy(ch)) ; return byte; } ``` Although probably the proper way to handle this is to remove the volatile on `byte` and instead add some memory fence before `return`. So either C11/C++11's [atomic_signal_fence()](https://en.cppreference.com/w/c/atomic/atomic_signal_fence) or (the undocumented) `__compiler_memory_barrier()`.

kilograham commented

2021-05-27 12:11:48 -05:00

(Migrated from github.com)

Yes, in the above example, removing the volatile in function f will always cause it to return 42 (i.e. declaring the second arg to dma_channel_transfer_to_buffer_now a pointer to volatile doesn't help) but without the change in declaration, the above fixed code requires a cast

Yes, in the above example, removing the `volatile` in function f will always cause it to return 42 (i.e. declaring the second arg to `dma_channel_transfer_to_buffer_now` a pointer to volatile doesn't help) but without the change in declaration, the above _fixed_ code requires a cast

kilograham commented

2021-05-31 12:41:48 -05:00

(Migrated from github.com)