Feat/nrf52xxx/spi/improve #4699
Conversation
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
| var freq uint32 | ||
| switch { | ||
| case config.Frequency == 0: // default MCU SPI speed | ||
| freq = nrf.SPIM_FREQUENCY_FREQUENCY_M4 |
There was a problem hiding this comment.
I don't see this as improvement :( Previous code looks cleaner to me: separates concerns of a) ensuring default value and b) handling frequency.
There was a problem hiding this comment.
Found the separate if-statement superfluous, since we already have a flow control on the frequency.
There was a problem hiding this comment.
can you two @ysoldak, @b0ch3nski agree on that?
There was a problem hiding this comment.
I find switch statements much easier to read but I think it's just a matter of personal preference. Nevertheless, we already have a switch here and that lonely if statement looks like it has rejoined it's people 😃
There was a problem hiding this comment.
I find switch statements much easier to read but I think it's just a matter of personal preference. Nevertheless, we already have a switch here and that lonely if statement looks like it has rejoined it's people 😃
Yes, felt the same to me.
There was a problem hiding this comment.
Regarding switch, I must confess, I was struggling to understand what's going on at first.
I was expecting every 'case' to produce a unique value (number of unique values = number of cases in switch).
I would propose merge two cases, but then case condition becomes long and look ugly.
So I still prefer deal with default value first, then handle the value, regardless default it is or not.
It may look nice/smart to have only one switch without an extra "if" before it, because we can, but I'd say readability suffers, at least in my case.
| // Read implements [io.Reader]. And reads as many bytes as the given buffer is long | ||
| func (spi *SPI) Read(r []byte) (int, error) { | ||
| return len(r), spi.Tx(nil, r) | ||
| } | ||
| | ||
| // Write implements [io.Writer]. And writes as long as there are bytes in w. | ||
| func (spi *SPI) Write(w []byte) (int, error) { | ||
| return len(w), spi.Tx(w, nil) | ||
| } | ||
| |
There was a problem hiding this comment.
Read and Write are not part of the SPI interface. It is easy enough to implement an abstraction if needed that works for all SPI hardwares of all MCUs
func writeSPI(w []byte, spi interface {Tx(w,r []byte) error}) (int,error) { return len(w), spi.Tx(w,nil) }There was a problem hiding this comment.
Yeah, lets do it! Actually I was surprised that SPI is not supplying it, since it would have seemed the more natural way to interface with SPI than the current one. Can you point me to the relevant file/code section?
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
milkpirate force-pushed the feat/nrf52xxx/spi/improve branch from 65e67b9 to 6163664 Compare 
There was a problem hiding this comment.
There are a few different things this PR does, which I think should be done separately. Some I agree with, some I don't. Especially since it affects the public API. Example: I think making the SPI type a pointer receive is a good idea, but that should probably be done for all SPI implementations at the same time to ensure consistency. And be done separate from other changes, like the mode constants (which, again, should be done for all chips at the same time in my opinion).
5 participants
As requested per #4660 (review)
What I did
SPI