Simon: Regarding hal_spi_txrx(), the comment block above the prototype specifically addresses your question. Not sure what you are looking at, but my version of hal_spi.h has this in it:
* MASTER: master sends all the values in the buffer and stores the * stores the values in the receive buffer if rxbuf is not NULL. * The txbuf parameter cannot be NULL. * SLAVE: cannot be called for a slave; returns -1 That seems pretty explicit to me: txbuf cannot be NULL; rxbuf can be NULL. Regarding the issues with some ports you might be correct and it may be a bug in a particular MCU implementation. I do not have specific experience with stm hal spi so would have to look at that code to be sure. My only other comment is the removal of hal_spi_tx_val(). I would not remove this api. The current word sizes supported are 8 and 9 bits so the hal does not currently support 16-bit spi transfers. At least, I do not think it does. I agree that it would be ambiguous if that were the case but given that it is not currently supported I do not see a need to remove this api. I would also add that if 16-bit spi transfers get supported this api remains and it simply is ignored or causes an assert (or some such behavior). > On Dec 20, 2018, at 8:30 AM, Simon Frank <[email protected]> wrote: > > Hello! > Some questions and suggestions regarding the SPI interface. > > > 1. What is the expected behavior of hal_spi_txrx()? > --------------------------------------------------- > > From hal_spi.h: > ``` > /** > ... > * @param spi_num SPI interface to use > * @param txbuf Pointer to buffer where values to transmit are stored. > * @param rxbuf Pointer to buffer to store values received from peer. > * @param cnt Number of 8-bit or 16-bit values to be transferred. > * > * @return int 0 on success, non-zero error code on failure. > */ > int hal_spi_txrx(int spi_num, void *txbuf, void *rxbuf, int cnt); > ``` > > - Can `txbuf` and/or `rxbuf` be NULL? I suspect this is handled in some nrf > ports but it do not seem to work with stm32 ports. It is not clear from the > comment whatever this is a bug in the stm32 port or expected behavior. > (The comments seems like a good place to put this sort of info) > > - why is `txbuf` param not const? Is a implementation allowed to modify this > data? I suspect not!? Adding const will probably not be seen in the > assembler > output but makes the expected behavior clear. It also allows higher > abstraction layers to use const without ugly casting. > > > 2. Error codes from hal functions? > ---------------------------------- > > I do not know if this have been discussed before, but is (non-zero) error > codes > from hal functions supposed to be same for all ports? I have noticed that > ST HAL error codes is sometimes converted to -1, but this seems like throwing > away useful debug information (a "magic" non-zero error code in the log is > better > then nothing). > > > 3. Suggestion: remove hal_spi_tx_val() > -------------------------------------- > > This function returns 0xffff on error which seems like a ambiguous result > when 16 bit spi used. It is also reduntant to hal_spi_txrx(). > > Backward compatible function: > ``` > // deprecated > static inline uint16_t hal_spi_tx_val(int spi_num, uint16_t txval) > { > uint16_t rxval = 0; > return hal_spi_txrx(spi_num, &val, rxd, 1)) ? 0xFFFF : rxval; > } > ``` > > 4. Suggestion: remove hal_spi_data_mode_breakout() > -------------------------------------------------- > > Redundant function as the information is already encoded as follows: > ``` > #define HAL_SPI_MODE_CPHA_MSK (0x1) > #define HAL_SPI_MODE_CPOL_MSK (0x2) > /** SPI mode 0. CPOL=0, CPHA=0 */ > #define HAL_SPI_MODE0 (0) > /** SPI mode 1. CPOL=0, CPHA=1 */ > #define HAL_SPI_MODE1 (HAL_SPI_MODE_CPHA_MSK) // 1 > /** SPI mode 2. CPOL=1, CPHA=0 */ > #define HAL_SPI_MODE2 (HAL_SPI_MODE_CPOL_MSK) // 2 > /** SPI mode 3, CPOL=1, CPHA=1 */ > #define HAL_SPI_MODE3 (HAL_SPI_MODE_CPOL_MSK | HAL_SPI_MODE_CPHA_MSK) // 3 > ``` > i.e. all HAL_SPI_MODEn values same as before. > > Backward compatible function (and usage example): > ``` > // deprecated > static inline int hal_spi_data_mode_breakout(uint8_t mode, > int *out_cpol, int *out_cpha) > { > *out_cpol = mode & HAL_SPI_MODE_CPOL_MSK; > *out_cpha = mode & HAL_SPI_MODE_CPHA_MSK; > return 0; > } > ``` > > Best Regars // Simon F.
