implement FullDuplex for Spi

[dkm]

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]

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

[dkm]

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]

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

[dkm]

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) ?

[matoushybl]

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 ?

[dkm]

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.

[therealprof]

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.

[dkm]

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

[dkm]

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]

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))

[dkm]

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 ?

[therealprof]

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

[matoushybl]

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.

[dkm]

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 ?

[therealprof]

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. ;)

[matoushybl]

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

[dkm]

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]

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

[dkm]

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]

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]

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

[dkm]

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]

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]

even better, I have some brand new f4 here!

[easybe]

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

[therealprof]

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

[easybe]

OK, I should be able to do that.

[dkm]

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.