Thanks! I am obviously blind and missed that part! Sorry! ---- will sanfilippo wrote ----
>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. >
