Feat/nrf52xxx/spi/improve #4699

milkpirate · 2025-01-14T21:44:44Z

As requested per #4660 (review)

What I did

SPI

Create SPIMode type
Make SPI implement io.Reader, io.Writer
Update formatting to align with other switch-cases
Update SPI documentation reference

Signed-off-by: Paul Schroeder <[email protected]>

ysoldak · 2025-01-17T14:27:28Z

src/machine/machine_nrf52xxx.go

-		conf &^= (nrf.SPIM_CONFIG_CPOL_ActiveHigh << nrf.SPIM_CONFIG_CPOL_Pos)
-		conf &^= (nrf.SPIM_CONFIG_CPHA_Leading << nrf.SPIM_CONFIG_CPHA_Pos)
-	}
+	conf = config.Mode.ApplyTo(conf)


I don't feel this is necessary?
SPIMode type 👍
ApplyTo() method 👎

I like it because:

It says what it does. It applies the mode to the config

It make the switch case testable

It hides the implementation details which are not necessary at this level of logic

Though they are still inspect-able if one is really interested

I would like to keep it in if you have not striking counter arguments. I can also make it private if you dont like it being public.

This change is a machine level exported API change, which is not in itself a bad change but it must be discussed beforehand. A lot of things must be checked before making such a change.

It hides the implementation details which are not necessary at this level of logic

Configure takes a SPIConfig and configures hardware. There is no intermediate step to abstract. There is no shared logic here between other MCUs. "Not necessary" are too strong words. Maybe one could argue that modularizing functions within Configure allows sharing logic between other higher level functions; but that is not the case here. The only argument I see for this change is that Configure is too long of a function to comfortably read- and even in that case I'd argue: on what basis? There seems to be ~6 actions performed within Configure which add to the cognitive load- and your change does not actually reduce the amount of actions performed, it just shortens the code, which might make it easier to read but does not reduce the intrinsic amount of congitive load of the Configure method.

I'd strongly err on the side of avoiding changes that make code less localized without reducing cognitive load.

Test ability is no argument for you, ok. Will inline.

ysoldak · 2025-01-17T14:29:56Z

src/machine/machine_nrf52xxx.go

 	// set frequency
 	var freq uint32
 	switch {
+	case config.Frequency == 0: // default MCU SPI speed
+		freq = nrf.SPIM_FREQUENCY_FREQUENCY_M4


I don't see this as improvement :( Previous code looks cleaner to me: separates concerns of a) ensuring default value and b) handling frequency.

Found the separate if-statement superfluous, since we already have a flow control on the frequency.

can you two @ysoldak, @b0ch3nski agree on that?

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 😃

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.

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.

soypat

machine package breaking changes must be discussed beforehand. And some nits

soypat · 2025-01-18T13:58:23Z

src/machine/machine_nrf52xxx.go

-		conf &^= (nrf.SPIM_CONFIG_CPOL_ActiveHigh << nrf.SPIM_CONFIG_CPOL_Pos)
-		conf &^= (nrf.SPIM_CONFIG_CPHA_Leading << nrf.SPIM_CONFIG_CPHA_Pos)
-	}
+	conf = config.Mode.ApplyTo(conf)


This change is a machine level exported API change, which is not in itself a bad change but it must be discussed beforehand. A lot of things must be checked before making such a change.

It hides the implementation details which are not necessary at this level of logic

Configure takes a SPIConfig and configures hardware. There is no intermediate step to abstract. There is no shared logic here between other MCUs. "Not necessary" are too strong words. Maybe one could argue that modularizing functions within Configure allows sharing logic between other higher level functions; but that is not the case here. The only argument I see for this change is that Configure is too long of a function to comfortably read- and even in that case I'd argue: on what basis? There seems to be ~6 actions performed within Configure which add to the cognitive load- and your change does not actually reduce the amount of actions performed, it just shortens the code, which might make it easier to read but does not reduce the intrinsic amount of congitive load of the Configure method.

I'd strongly err on the side of avoiding changes that make code less localized without reducing cognitive load.

soypat · 2025-01-18T14:02:04Z

src/machine/machine_nrf52xxx.go

+// 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)
+}
+


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

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?

soypat · 2025-01-18T14:03:17Z

src/machine/machine_nrf52xxx.go

+	SPI2 = SPI{Bus: nrf.SPIM2}
+)
+
+type SPIMode uint8


This is a breaking change. It must be discussed in an issue beforehand.

Ok, will remove it from the config struct definition, so one can pass to the config but the type is not required.

soypat · 2025-01-18T14:04:10Z

src/machine/machine_nrf52xxx.go

-// Configure is intended to setup the SPI interface.
-func (spi SPI) Configure(config SPIConfig) error {
+// Configure is intended to set up the SPI interface.
+func (spi *SPI) Configure(config SPIConfig) error {


The pointer receiver is not a change I'm against, in fact I think it may make sense to change it. I'd be curious as to why you are changing it :)

Well the SPI interfaces are created as singletons, so why should we copy it on its receivers.

One would usually expect to configure the very instance where Configure is attached to and not a copy (that gets thrown away. Technically we configure the hardware not the instance but still it feels odd and as I said counter intuitive.

Signed-off-by: Paul Schroeder <[email protected]>

milkpirate added 4 commits January 14, 2025 22:29

feat(src/machine/nrf52xxx/spi): add mode

34bb9e8

Signed-off-by: Paul Schroeder <[email protected]>

feat(src/machine/nrf52xxx/spi): re-use case instead of dedicated

5c202bd

Signed-off-by: Paul Schroeder <[email protected]>

feat(src/machine/nrf52xxx/spi): implement io.Reader, io.Writer

08b2d01

Signed-off-by: Paul Schroeder <[email protected]>

chore(dependencies): remove residuals

e8aa554

Signed-off-by: Paul Schroeder <[email protected]>

This was referenced Jan 14, 2025

Feat/nrf52xxx/spi/tests #4700

Closed

feat(nrf52xxx/adc|spi): adjustable resolution, improve SPI #4660

Closed

ysoldak reviewed Jan 17, 2025

View reviewed changes

soypat requested changes Jan 18, 2025

View reviewed changes

chore(review): honor

6163664

Signed-off-by: Paul Schroeder <[email protected]>

milkpirate force-pushed the feat/nrf52xxx/spi/improve branch from 65e67b9 to 6163664 Compare January 18, 2025 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/nrf52xxx/spi/improve #4699

Feat/nrf52xxx/spi/improve #4699

milkpirate commented Jan 14, 2025 •

edited

Loading

ysoldak Jan 17, 2025

milkpirate Jan 17, 2025

soypat Jan 18, 2025

milkpirate Jan 18, 2025

ysoldak Jan 17, 2025 •

edited

Loading

milkpirate Jan 17, 2025

milkpirate Jan 18, 2025

b0ch3nski Jan 18, 2025

milkpirate Jan 18, 2025

ysoldak Jan 18, 2025

soypat left a comment

soypat Jan 18, 2025

soypat Jan 18, 2025

milkpirate Jan 18, 2025 •

edited

Loading

soypat Jan 18, 2025

milkpirate Jan 18, 2025

soypat Jan 18, 2025

milkpirate Jan 18, 2025

Feat/nrf52xxx/spi/improve #4699

Are you sure you want to change the base?

Feat/nrf52xxx/spi/improve #4699

Conversation

milkpirate commented Jan 14, 2025 • edited Loading

What I did

SPI

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

ysoldak Jan 17, 2025 • edited Loading

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

soypat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milkpirate Jan 18, 2025 • edited Loading

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

milkpirate commented Jan 14, 2025 •

edited

Loading

ysoldak Jan 17, 2025 •

edited

Loading

milkpirate Jan 18, 2025 •

edited

Loading