SPI default write value not respected by async SPI

[multiplemonomials]

Description of defect

Recently, I've had the "pleasure" of working with a sensor that absolutely requires 0x0 as an SPI fill character (the dummy data sent when doing a read-only transaction). If this is not satisfied, it will throw a fit and reset itself. This led me to the discovery that Mbed's SPI API does not respect the fill character that you set (with SPI::set_default_write_value()) for asynchronous SPI transactions. Looking into it, I found that STMicro's HAL is hardcoded to use 0xFF for the fill character, but it's not their fault: the data value that is set by SPI::set_default_write_value() (SPI::_write_fill) is simply not passed down to the HAL at all for async transfers. So, it has no way of knowing what the user wants for the fill character.

Hopefully, this can be changed in the future so that the fill character is passed down to the HAL (maybe through spi_init()?) and the HAL respects this value. That would make life a lot easier for me!

Target(s) affected by this defect ?

All that implement async SPI. Seen on the NUCLEO_F429ZI.

Toolchain(s) (name and version) displaying this defect ?

GCC_ARM

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.1.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

Not relevant here

How is this defect reproduced ?

• Create an SPI object
• Call spi.set_default_write_value(0x00)
• Start an asynchronous read using SPI::transfer() with a null transmit buffer
• Observe with a logic analyzer that MOSI is set high during the entire transfer (should be low for zeros)

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers. Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2851

[0xc0170]

I would have thought this was passed down to the async as I recall adding the fill character to SPI. Easy fix but it will require to update all targets that support async 👀

[pea-pod]

I did some digging on this because I thought I could be smart and pick off some low hanging fruit. That should have been a clue (me being smart).

On looking at the code for a while, I decided to implement this by adding the fill value to the ST spi_s structure, but as I went up layers, I realized that there was no function supplied by the C++ drivers or the C HAL that could pass the fill value downward.

I found this in the stm_spi_api.c file:

int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length,
                           char *rx_buffer, int rx_length, char write_fill)

which then calls the appropriate functions and sets the default write fill char in the transfer. But of course, this is the fully blocking method. When you look at the call to the asynchronous version in stm_spi_api.c:

void spi_master_transfer(spi_t *obj, const void *tx, size_t tx_length, void *rx,
    size_t rx_length, uint8_t bit_width, uint32_t handler, uint32_t event, DMAUsage hint)

You'll notice that the fill_value does not get passed down to it. Indeed, as far as I can tell, there is no way to pass the fill value down to the C API that each vendor implements, as that spi_t structure is vendor defined. Any code that modified it would have to exist in the SPI.cpp file and therefore, grow a lot of #if defined hair. This appears to me that it would tend to mix up concerns. I have looked at the Nuvoton and Nordic implementations, and neither of them seem to handle the fill value for the asynchronous transfer method either.

The spi_master_block_write (i.e. the blocking) transfer method works fine and would seem to respect the fill_value method, but that implementation is essentially calling the single value write function over and over until it finishes, at least in the ST version as well as several others.

The ST HAL does not improve matters much, because it already assumes that your input buffer is filled with whatever the correct characters are already. The call to HAL_SPI_Receive_IT essentially does some housekeeping (setting the tx buffer pointer to the rx buffer pointer location) and then calls the HAL_SPI_TransmitReceive_IT function. I only checked the L4 version for this, so it could be different in a different device family. That being said, that does not help if you have the L4, so it does not seem like there is a good method.

It seems like there are a handful of ways to address this:

1. Do a memset call in the C++ driver. This would be an additional wait time for the set operation, even if DMA is used on the lower layers.
2. Another C function that actually includes writing the dummy value so that it can be sent to the vendor functions. This could be a weakly defined function that, if not overridden, could simply call the current transfer method, but drop the fill_value, perhaps with a #warning.
3. Add a new function to the C SPI HAL that set the fill value as appropriate and supplied a pointer to an spi_s structure. This would require further work to add the right method calls per member, but at least there's a chance it could be implemented.
4. Clarify in documentation that the SPI::_fill_value member is only for the SPI::write method, and not the semi-asynchronous SPI::transfer method.

[jeromecoutant]

Hi @multiplemonomials @pea-pod Should we keep this issue opened ?

[pea-pod]

It's definitely an issue still.

I tried a couple of months ago to take the current master and rebase it onto the current SPI HAL update. It was amazingly painful and I probably gave up 3 merges short of the complete rebase. I write all this just to say that I have tried and it was not a minimal effort, so I was not sure what the right path forward is.

I am ok with whatever, but it would be nice if somehow we could move forward with updating the SPI code in general.

[0xc0170]

@MarceloSalazar please review

[MarceloSalazar]

@multiplemonomials thanks for raising this issue and providing feedback on possible ways to resolve it. I've marked this ticket as feedback to the requirements we're collecting for an improved version of the HAL.

[ciarmcom]

We closed this issue because it has been inactive for quite some time and we believe it to be low priority. If you think that the priority should be higher, then please reopen with your justification for increasing the priority.

[multiplemonomials]

I believe that this issue is worth keeping open, because it's a case where the API is not consistent with itself. I don't seem to have the ability to reopen it though.

[multiplemonomials]

Is this ever going to be reopened?