implement FullDuplex for Spi #122

dkm · 2020-11-03T19:44:45Z

This is a very simple impl for FullDuplex. The Transfer impl could be removed in favor of the default that is available by the availability of FullDuplex.

dkm · 2020-11-03T19:53:37Z

Looks like it's not working, will push a correct version later :)

dkm · 2020-11-03T20:05:10Z

Not 100% sure it's the best way to implement, but as far as I could test, it works (tested with ws2812-spi and a 10 leds chain).
If you think some refactoring can be done, do not hesitate, happy to learn and implement :)

matoushybl · 2020-11-10T19:24:34Z

src/spi.rs

+    fn send(&mut self, byte: u8) -> nb::Result<(), Error> {
+        self.check_send()?;
+        self.send_u8(byte);
+        self.check_send().ok();


Why are you discarding the result of the check_send the second time?

I've simply reused the code from the write that does the same thing. You are right that it should raise any error instead of discarding it. Maybe this is also true in write (same file) ?

I see, we should check the datasheet to see if the status register needs to be read after writing the data to the data register.
I believe that the propose of this construct is to check the status of errors such as Overrun, etc. so I propose that we should implement method check_errors that only checks for the errors and then propagate its result using the ? operator.

What do you think, @therealprof ?

Looks like a good idea. If you think it's worth implementing, I can do it :) If you have other idea (eg. refactoring), I would be happy to help.

Seems like a good plan, we might want to be careful about which errors we propagate and which ones we drop but we can look at that later when we have the check_errors function I guess.

Ok, I'll draft something and add it here :)

dkm · 2020-11-15T13:20:52Z

I've tried to implement the check_errors. I'm not 100% sure my usage of nb::Result / Result is the best (or at least the more idiomatic). I've made it return a Result as it's not dealing with blocking anymore, but maybe that was not a good idea. Let me know if you think I should do it differently !

therealprof · 2020-11-15T20:19:23Z

src/spi.rs

+        match self.check_errors() {
+            Ok(_) => Ok(()),
+            Err(e) => Err(nb::Error::Other(e)),
+        }


Couldn't this be written more obviously as:

Suggested change

match self.check_errors() {

Ok(_) => Ok(()),

Err(e) => Err(nb::Error::Other(e)),

}

self.check_errors().map_err(|e| nb::Error::Other(e))

I thought of that, but it seems you can only use it to change the error, not the Result type wrapping everything (Result vs nb::Result here). I get this:

rror[E0308]: mismatched types --> src/spi.rs:482:9 | 475 | fn send(&mut self, byte: u8) -> nb::Result<(), Error> { | --------------------- expected `core::result::Result<(), nb::Error<spi::Error>>` because of return type ... 482 | self.check_errors().map_err(|e| Err(nb::Error::Other(e))) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `nb::Error`, found enum `core::result::Result`

But maybe I'm not doing something correctly ?

Sorry, my bad. Cut'n'pasto. Of course it needs to be (corrected above, too):

self.check_errors().map_err(|e| nb::Error::Other(e))

map_err() automatically wraps the value returned from the closure in an Err, cf. here: https://doc.rust-lang.org/src/core/result.rs.html#592

I believe that the map_err isn't needed at all. Check for example the check_send method, where check_errors is called directly with the ? operator. There might be an automatic Into trait implementation for it.

It works with ? yes (and I was surprised), but not if using the statement for returning (ie. without ?). I'll fix the map_err except if you have something better in mind ?

I believe that the map_err isn't needed at all. Check for example the check_send method, where check_errors is called directly with the ? operator. There might be an automatic Into trait implementation for it.

Depends on the signature. If we're replying the right Result type then we can just use it directly. Indeed we can also use ? if we have proper conversion implementations available, however IIRC using ? as the last operation in a function is frowned upon and linted by clippy. ;)

Oh, sorry, I missed that it was the last command in function. LGTM then with map_err.

dkm · 2020-11-16T11:28:09Z

Thanks for the help !
Also, I've left the Transfer impl as relying on the FullDuplex would cause set_bidi to be called for every byte in a mult-byte transfer... Not sure if this overhead is a problem or not, so I left the custom Transfer impl as-is.

therealprof · 2020-11-17T19:27:23Z

Has anyone had a chance to test those changes in real life yet?

dkm · 2020-11-19T10:14:43Z

I guess I could hookup a nRF24L01, capture some exchanges with analyzer to provide some better test if nobody can test this with an easier setup ? I don't have anything else (except for the ws2812 I'm using that motivated this change)

dkm · 2020-12-09T11:15:33Z

Sorry, I forgot to think this through. I don't have anything to test besides this ws2812 setup as all my F0 are soldered on my keyboards, so there's no way I can monitor sck and miso pins...

matoushybl · 2020-12-10T09:38:48Z

If you design a test scenario, I have a lot of hardware to test on.

dkm · 2021-01-13T17:27:34Z

Sorry, I forgot to think this through. I don't have anything to test besides this ws2812 setup as all my F0 are soldered on my keyboards, so there's no way I can monitor sck and miso pins...

If you design a test scenario, I have a lot of hardware to test on.

back on this... I was hoping I could find some f0, but found none :/.
I can try to write a small program so you only need to wire something, compile and load ? Do you have some nrf24lo1 ? I could execute the program on another mcu to check it works and let you see if it's works similarly on f0?

matoushybl · 2021-01-14T16:22:03Z

Sure, no problem. I believe that I have the NRF so there should be no problems.

I believe that the peripheral on F4 is similar, so that you can debug on it and then send it to me to try out on the F0.

dkm · 2021-01-14T17:07:34Z

even better, I have some brand new f4 here!

check_errors only checks for Overrun, CRC error and ModeFault flags. Signed-off-by: Marc Poulhiès <dkm@kataplop.net>

easybe · 2022-10-23T11:34:46Z

Hi. What is the status on this? Can I help in any way?

therealprof · 2022-10-23T11:48:30Z

@easybe Seems dormant to me. If you can test it, we can certainly get it merged.

easybe · 2022-10-23T12:08:09Z

OK, I should be able to do that.

dkm · 2022-10-23T12:30:13Z

Sorry, still haven't anything to test, but am using the code daily in a keyboard without any issue, so should be OK. Thanks Ezra if you can help with the testing.

dkm closed this Nov 3, 2020

dkm reopened this Nov 3, 2020

dkm force-pushed the master branch from 7d2eec6 to cb5dbf2 Compare November 3, 2020 20:03

matoushybl reviewed Nov 10, 2020

View reviewed changes

dkm force-pushed the master branch 8 times, most recently from d9a3f0b to f890bcc Compare November 15, 2020 13:18

therealprof reviewed Nov 15, 2020

View reviewed changes

dkm force-pushed the master branch from f890bcc to 34d2ee3 Compare November 16, 2020 11:25

Implement FullDuplex for SPI and add check_errors method

68b434a

check_errors only checks for Overrun, CRC error and ModeFault flags. Signed-off-by: Marc Poulhiès <dkm@kataplop.net>

dkm force-pushed the master branch from 34d2ee3 to 68b434a Compare February 25, 2022 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement FullDuplex for Spi #122

implement FullDuplex for Spi #122

dkm commented Nov 3, 2020

dkm commented Nov 3, 2020

dkm commented Nov 3, 2020

matoushybl Nov 10, 2020

dkm Nov 10, 2020

matoushybl Nov 11, 2020

dkm Nov 11, 2020

therealprof Nov 14, 2020

dkm Nov 14, 2020

dkm commented Nov 15, 2020

therealprof Nov 15, 2020 •

edited

dkm Nov 15, 2020

therealprof Nov 16, 2020

matoushybl Nov 16, 2020

dkm Nov 16, 2020

therealprof Nov 16, 2020

matoushybl Nov 16, 2020

dkm commented Nov 16, 2020

therealprof commented Nov 17, 2020

dkm commented Nov 19, 2020

dkm commented Dec 9, 2020

matoushybl commented Dec 10, 2020

dkm commented Jan 13, 2021

matoushybl commented Jan 14, 2021

dkm commented Jan 14, 2021

easybe commented Oct 23, 2022 •

edited

therealprof commented Oct 23, 2022

easybe commented Oct 23, 2022

dkm commented Oct 23, 2022 via email

implement FullDuplex for Spi #122

Are you sure you want to change the base?

implement FullDuplex for Spi #122

Conversation

dkm commented Nov 3, 2020

dkm commented Nov 3, 2020

dkm commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkm commented Nov 15, 2020

therealprof Nov 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkm commented Nov 16, 2020

therealprof commented Nov 17, 2020

dkm commented Nov 19, 2020

dkm commented Dec 9, 2020

matoushybl commented Dec 10, 2020

dkm commented Jan 13, 2021

matoushybl commented Jan 14, 2021

dkm commented Jan 14, 2021

easybe commented Oct 23, 2022 • edited

therealprof commented Oct 23, 2022

easybe commented Oct 23, 2022

dkm commented Oct 23, 2022 via email

therealprof Nov 15, 2020 •

edited

easybe commented Oct 23, 2022 •

edited