libraries: fixed SPI

[thangktran]

to handle other bit lengths rather than 8.
Thanks to @UlliBien for the fix suggestion.

Fixes: #2820

[earlephilhower]

Thanks, @thangktran. I can't test this, but it seems fine logically.

The problem originated because we had a write16() call and a transfer16() call, both of which did the same thing (except for the value return).

I think it makes sense here to remove the write16() method guts and replace it with a call to (void) transfer16() (the (void) to be explicit that we're ignoring the returned value).

This way we only have one version of the actual work code, which seems like good code hygiene. I haven't gone thru the rest, but if there's a writeByte() and a transferByte(), the same comment applies...

[thangktran]

Thanks, @thangktran. I can't test this, but it seems fine logically.

The problem originated because we had a write16() call and a transfer16() call, both of which did the same thing (except for the value return).

I think it makes sense here to remove the write16() method guts and replace it with a call to (void) transfer16() (the (void) to be explicit that we're ignoring the returned value).

This way we only have one version of the actual work code, which seems like good code hygiene. I haven't gone thru the rest, but if there's a writeByte() and a transferByte(), the same comment applies...

Thank you for your input @earlephilhower .
I just scanned through the code and find out that there are lots of code duplication between transfer*() method and write*() method.
I would open an Issue an prepare a PR to do these refactoring since they're not related to this bugfix branch/commit.
What is your opinion?

[earlephilhower]

Looks good. We'll do the cleanup (transfer/write de-duplication) in another issue/PR. Thx!

[thangktran]

Is this PR ready-to-be-merged? I need it for #6425

[UlliBien]

@devyte:

This change is forcing 16bit transfer. I think the "16" in transfer16() refers to the data type taken as arg, i. e. uint16_t, and not the transfer bit length. With this change it is no longer possible to send a 16 bit value as 2 8bit values with configured byte ordering, unless the user does 2 method calls.

That's what intended! transfer16() should transfer 16 bits (a word) with bit order for the complete word. If you want to transfer two bytes call transfer() twice in that byte order you want.
There are devices that need 16 Bit transfers. RFM12 e.g. And if you want to transfer a 16 or 32 bit address you have to transfer the bits as a unit and not as a set of bytes.

Without the change you get:
CS singal controlled by the user:

  digitalWrite(CS, LOW); // CS goes low
  transfer16(x);         // 16 bit transfer without change of CS
  digitalWrite(CS, LOW); // CS goes high

Everything works as expeted.

Now let the hardware control the CS signal:

setHwCs(true);
...
transfer16(x); // CS goes low
               // 8 Bit transfer
               // CS goes high
               // CS goes low
               // 8 Bit transfer
               // CS goes high

Do you see the difference?

The change does not effect old code that does not use the hardware control. Byte order and bit oder isn't changes. The only difference is the behavior of the CS signal.

[devyte]

The change affects any app that currently uses transfer16(), and transfer16() has been around for a while. My main concern is that this PR introduces a change in behavior which could affect compatibility.

Receiving an arg uint16_t is one thing, and transmitting a 16bit value is something completely unrelated. You can have a transfer16() that receives a uint16_t argument, and transmit it either as 2 individual bytes or as a 16bit value, depending on the setDataBits() setting, or on some member bool in the SPI class.

Is the current code illegal SPI? Does it not work? Is there s device with which it does work?

You said:

There are devices that need 16 Bit transfers

I understand that. But what about the opposite? Do all devices understand 16bit transfers? Is there a device that does not understand 16bit transfers, but to which you would like to send a 16bit value as 2 8bit transfers?
If the answer is yes, then the current code is also valid and shouldn't be dropped. As I said, my view is that receiving a uint16_t as function argument and sending a 16bit transfer are two different independent things. In that case, I would think either a new function is needed, or there should be a new setting (i. e. a bool) thst cbooses between the old and new code.

To be clear, I'm not saying such a bool should be added and be done with it. I'm saying it is not clear to me that the current code should be dropped in favor of the proposed code, because so far I haven't seen a convincing argument that the current code is invalid.

You said:

The change does not effect old code that does not use the hardware control

So then that means that the behavior for hw vs. sw CS is not the same with this change. Enable hw CS, and transfer16 toggles CS between bytes. Does it make sense to have such different behavior? I would think that if I change the CS pin used in my circuit, and switch to sw CS control, the transmission bits within the SPI functions would remain the same! Then again, as I said, I'm not an SPI expert.

[devyte]

I'm still not convinced that the current code is invalid. However, I also don't want to hold up your SPI work*.
How about this: I merge this PR as it is now, and if somebody complains about broken 16bit transfers I will @ you to step in and coordinate fixing whatever is needed?

*The comments in the discussion that was pointed at are valid. If the hw supports more than one CS, an enhancement proposal would be welcome. Comments in the header would help understand the SPI class methods better and avoid having to look at the cpp. Precalculation. Etc.

[earlephilhower]

I'm not able to find a "definitive" spec for it, but from what I've seem at TI and uChip, CS is asserted for the length of the transfer for all lengths of comms:
https://wiki.analog.com/resources/eval/sdp/sdp-b/peripherals/spi

CS deassertion looks like it is supposed to reset the slave's state machine, but all I've been able to find are non-definitive verilog (read: some guy on the internet's code) so that's not really canon.

[thangktran]

I'm still not convinced that the current code is invalid. However, I also don't want to hold up your SPI work*.
How about this: I merge this PR as it is now, and if somebody complains about broken 16bit transfers I will @ you to step in and coordinate fixing whatever is needed?

*The comments in the discussion that was pointed at are valid. If the hw supports more than one CS, an enhancement proposal would be welcome. Comments in the header would help understand the SPI class methods better and avoid having to look at the cpp. Precalculation. Etc.

@devyte just to be clear. So we have transfer16() and write16() is on purpose due to the way they behave and not code duplication?

[devyte]

@thangktran not clear, that is part of my point. What you say is one possibility.
Another off the top of my head is to have overloads, e. g. :

transfer(uint8_t arg); //single byte, 8bit mode
transfer(uint16_t arg); //16bits, 8bit mode
transfer16(uint8_t arg); //single byte 16bit mode
transfer16(uint16_t arg); //16bits, 16bit mode

[devyte]

Yet another possibility is to have a transferWidth flag that defines the number of bits of the transfer mode. It would default to 8bits, but could be set to 16bits for 16bit transfer.

[Makuna]

Would it not make sense that this is a "transaction" setting? So SPISettings should be extended to include dataWidth?

[thangktran]

Would it not make sense that this is a "transaction" setting? So SPISettings should be extended to include dataWidth?

@Makuna This is a good idea. Thank you.

@devyte what is your opinion about @Makuna's approach?

[devyte]

Looking at the SPISettings class members, a _dataWidth addition looks right at home there. Whatever else, I think that should be done, preferably in its own PR.

That still leaves this PR, where the question is which approach to use for write/transfer. I see the following options (please correct me if I'm wrong):

1. Overload methods. The arg type defines how many bytes are to be transferred, the method postfix defines the transfer width. Example : transfer16(uint32_t arg) would transfer 4 bytes as 2 successive 16bit transfers.
2. A class flag or mode setter, which would change the transfer width for all calls after that. Example: SPI.setTransferBits(blah).
3. An additional arg to the current methods, similar to msb. Example: SPI.transfer16(val, true, width16).

[UlliBien]

Nobody needs such things! The current implementation is far away from the Arduino standards. Don't make it worse.
Please reead SPI specs and look how SPI devices work.

[Makuna]

I strongly believe that all three options are incorrect. I think this pull is going in the wrong direction to fix the issue. Please correct me if I get this wrong (I may have missed something). The API has two issues that are in conflict. One is that Esp specific issue of the pointer being on 8bit/16bit/32bit boundary. The other a SPI issue of sending "transactions" (SPI specific term here) of different bit width. The SPI API is using SpiSettings is used to configure the "transaction"; and I believe this should be adhered to. There should be no other way to set the bit width of a transaction. This then leaves the Esp specific "16" methods to be left alone to be used to signify the source data size.

[devyte]

@UlliBien Arduino is not a standard, it's a reference. Please be aware of the difference.

@Makuna from looking at the Arduino reference api docs, I see the following:

1. transactions don't support 16bit width
2. transfer() means take an 8bit value and transfer in 8bit width
3. transfer16() means take a 16bit value and transfer in 16bit width

There is no support for other cases. To me, that looks like an incomplete api. It certainly looks like we already support a broader range of cases.

[devyte]

One is that Esp specific issue of the pointer being on 8bit/16bit/32bit boundary

That isn't quite right. The discussion is about the argument type passed to the transferXX() functions vs. the XX in tje function name. Currently there is this:

transfer(uint8_t arg); //single byte arg, 8bit transfer width
transfer16(uint16_t arg); //16bit arg, 2x 8bit transfer width

The Arduino reference for SPI says nothing of 16bit transfer width, only of a 16bit val to send/receive.

My overloaded method proposal is this:

transfer(uint8_t arg); //single byte arg, 8bit transfer width 
transfer(uint16_t arg); //16bit arg, 2x 8bit transfer width 
transfer16(uint8_t arg); //single byte arg, 16bit transfer width
transfer16(uint16_t arg); //16bit arg, 16bit transfer width

Functionally, this contains our current functionality plus the proposal in this PR, and covers the Arduino reference ambiguity and all, i. e. it's a superset.

The other a SPI issue of sending "transactions"

As I said, the Arduino reference says nothing about 16bit width for transactions (unless I missed something). Still, I think the suggestion of adding the setting as a member in SPISettings is sound. Again, it would be a superset of the Arduino reference.

[Makuna]

Thank you both for making the issues clear to me.

I still feel this is the wrong direction, the nuance of passing a byte versus 16 bit could be too easy to miss the size difference and the nuance of what it means is not obvious when looking at the API. This screams bad API design. This is why I made the suggestion I did.

While often we find the Arduino "reference" vague and not well thought out, I feel this would not make it better; or even this API better. It feels like it would be harder to maintain and will require more future work. Let alone more people asking questions on the gitter channel and forums ;-)

[thangktran]

Thank you both for making the issues clear to me.

I still feel this is the wrong direction, the nuance of passing a byte versus 16 bit could be too easy to miss the size difference and the nuance of what it means is not obvious when looking at the API. This screams bad API design. This is why I made the suggestion I did.

While often we find the Arduino "reference" vague and not well thought out, I feel this would not make it better; or even this API better. It feels like it would be harder to maintain and will require more future work. Let alone more people asking questions on the gitter channel and forums ;-)

@Makuna I agree with you.
I think that if the user wants to send a 16bit data as 2 single-8-bit transfer, they should split the 16-bit data themself and call transfer(uint8_t arg) or write(uint8_t arg) twice.
I would propose that we keep transfer(uint8_t arg) and transfer16(uint16_t arg) for backward-compatibility and i would change it that they call write(uint8_t arg) and write16(uint16_t arg) internally for correct behaviour. What do you think @devyte @UlliBien ?