Stream x improve #666

YruamaLairba · 2023-07-03T21:43:31Z

Hi, i'm trying to rewrite the stream struct API, here is the list modification i'm doing on stream:

implement missing feature (already discussed in another Draft Pull Request)
remove unecessary generics (lready discussed elsewhere)
replace "class" method by "object" method to follow "peripheral ownership pattern"
remove "get" prefix on getter to follow coding style of rust compiler
rename things, to give a more natural, shorter or less ambiguous name
replace some raw parameter by enum to make thing safer or more natural
disable method just write 0 to the enable bit. In my opinion, trying to handle thing when disabling the hardware is the
role of an higher level abstraction

This is what i'm doing on Transfer struct
-adapt to make it compile with the modified stream API
-put the "disable" logic i removed from Stream
-EDIT add or fix method for interrupt flags manipulations, this is required to make i2c/dma module to compile again

EDIT: i2c/dma module use transfer and stream, so fixing it to fit the change

src/dma/mod.rs

burrbull · 2023-07-05T16:08:51Z

rebase, please and update examples if it is needed

YruamaLairba · 2023-07-06T18:17:19Z

can't rebase, there is conflicting change

burrbull · 2023-07-11T06:12:10Z

first squash commits and then rebase
See https://github.com/stm32-rs/stm32f4xx-hal/commits/StreamX_improve

YruamaLairba · 2023-07-11T11:26:22Z

will do, but ii need to fix examples first.

YruamaLairba · 2023-07-13T18:34:42Z

@burrbull do you know a way to check all examples ? when i use cargo check --examples --feature="stm32f411" only examples that don't require additional feature are checked. For the moment i just manually checked examples that contain "dma" in the name.

burrbull · 2023-07-13T18:39:51Z

if you on linux you could run ./tools/check.py to check all chips. Although it is slightly outdated (for example doesn't have rtic feature).

YruamaLairba · 2023-07-15T17:22:44Z

merging correctly will be difficult, there was many change since i started this PR, i fear Git will automatically resolve conflict in incoherent way. I have a example not working but I may forgot to check it earlier

YruamaLairba · 2023-07-16T17:54:56Z

@burrbull ready for a review, i just didn't change the CHANGELOG, do know what description i should put. I'm very skeptical Transfer. The main problem is the ownership taking of the peripheral, this prevent to freely use the peripheral. But beyond that, it's seems some people already added some functionality to it just to make it usable for some peripheral, which is again the intend of being general purpose stuff.

YruamaLairba · 2023-07-16T18:01:26Z

Sorry, I just found a typo i repeated many time because of automatic completion

src/dma/traits.rs

src/dma/mod.rs

src/dma/traits.rs

src/dma/mod.rs

src/i2c/dma.rs

therealprof

I can't really comment on the soundness of the DMA stuff but the approach looks good to me. 👍🏻

YruamaLairba · 2023-07-18T09:59:46Z

Since i Just started to use bitflag, i wonder if i should use it for all structure containing a set of boolean to keep coherence

YruamaLairba · 2023-07-18T10:27:23Z

crap, i missed the annoying "from_bits_retain" method in bitflags. This make my code unsound :(

YruamaLairba · 2023-07-18T14:45:20Z

@burrbull is it ok ? i think there may a lack of coherency with flags, but i found bitflag! a bit deceiving. The "from_bit_retain" method have disastrous consequence, and using the "Flag::A|Flag::B|Flag::C" is actually very verbose with concrete case:
"DmaCommonInterrupt::TRANSFER_COMPLETE|DmaCommonInterrupt::HALF_TRANSFER ..."

burrbull · 2023-07-18T16:17:52Z

ok lets leave bitflags for another PR.

YruamaLairba · 2023-07-18T17:57:52Z

@burrbull i already commited thing using bitflags

YruamaLairba · 2023-07-18T18:06:09Z

i may exaggerated a bit with "disastrous consequence" since i fixed the unsoundness. But i still think "from_bits_retain" is bad because of consequence it can have.

burrbull · 2023-07-20T04:16:23Z

@YruamaLairba could you look at/test #673 ?

YruamaLairba · 2023-07-22T10:39:38Z

@burrbull what is missing to merge this PR ? do wait for having a good system to manipulate a set of flags ?

burrbull · 2023-07-22T13:18:37Z

@burrbull what is missing to merge this PR ? do wait for having a good system to manipulate a set of flags ?

I'm fine to leave bit flags for some future PR.

But I don't see sense in duplicates (like DmaCommonInterrupts and DmaCommonInterrupts). It's confusing.
Or both DmaFlags and DmaCommonInterrupts are structs for read and write or both are bit flags.
Remove DmaCommonInterruptsMask.

YruamaLairba · 2023-07-22T20:14:32Z

So finally, i should just ignore what you did in the Flags branch and use structure with boolean field to manipulate several interrupts or flags at once ?

burrbull · 2023-07-23T03:10:16Z

So finally, i should just ignore what you did in the Flags branch and use structure with boolean field to manipulate several interrupts or flags at once ?

Yes

YruamaLairba · 2023-07-23T23:02:39Z

done; i also changed clear_all_flags(&mut self) by clear_flags(&mut self, flags:DmaFlags) and added some all() method to structure

burrbull · 2023-07-24T04:01:15Z

Ok. I'll merge this after fix release #674

burrbull · 2023-07-25T12:55:10Z

Add to CHANGELOG.md, please and rebase

YruamaLairba · 2023-07-29T15:59:07Z

Is it ok or a squash is required ?

burrbull · 2023-07-29T16:00:07Z

better to squash

add stream methods to set pincos, circ and pfctrl make Stream use dmaDirection instead generic direction make streams use a new DmaChannel enum instead generic stuff conditionnel compilation for DmaChannel enum Redesign ISR API of Stream, this break transfer redesign Stream trait API, break even more stuff implement Stream trait, many thing still broken fix most compile error in Transfer struct handle DmaDataSize relatd stuff stream_disable utility function use utility function for gracefully disable the transfer add/fix interrupt flag manipulation throught transfer type fix Bits impl for PeeripeheralIncrmentOffset fix bit impl of DmaFlowcontreoller fix missing unsafe section fix i2c dma to handle transfer API change. use Dmaflag for more expressive API dma common interrupts setter & getter fix spi-dma example fix rtic-spi-slave-dma.rs example fix bad link in the doc add number_of_transfer method on Transfer fix an example dma API, rename clear_smth_flag to clear_smth dma stream rename smth_flag -> is_smth dma stream rename set_smth_interrupt to listen/unlisten_smth, temporaly break transfer dma stream: listen/unlisten using mask, fix previous break fix, using bitflags for masking require to "truncate" some optimisation oops forgot to remove a TODO mark remove bitflags and mask stuff, use struct with bool field instead update CHANGELOG.md

YruamaLairba · 2023-07-29T17:53:39Z

squashed

burrbull reviewed Jul 4, 2023

View reviewed changes

src/dma/mod.rs Outdated Show resolved Hide resolved

YruamaLairba force-pushed the StreamX_improve branch 3 times, most recently from a4b8b13 to af0cc5e Compare July 16, 2023 16:19

YruamaLairba marked this pull request as ready for review July 16, 2023 16:27

YruamaLairba force-pushed the StreamX_improve branch from af0cc5e to 1e9826a Compare July 16, 2023 18:00