You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mynewt.apache.org by Sterling Hughes <st...@apache.org> on 2016/09/19 23:07:38 UTC
Notes from HAL discussions on sterly_refactor
Howdy:
We\u2019ve been going over a painstaking review of the various HAL APIs, to
make sure we capture all the changes we want to include prior to merge.
Today, we looked at the driver framework, hal GPIO and SPI, here are the
changes we discussed:
- GPIO needs to re-name the GPIO state enums to have an hal_* in the
name
- Driver functions in adc_dev and elsewhere should be static const to
save RAM.
- HAL GPIO: we should remove set/clear/toggle, and just have
hal_gpio_write(pin, val)
- HAL GPIO: there should be a function hal_gpio_release() so that ports
can be turned off when a pin is no longer used.
- Q: should hal_gpio_init_in/out follow the more common config(),
enable(), disable() paradigm in the rest of the HAL?
- HAL GPIO: hal_gpio_irq_enable() should by default clear any pending
IRQs. We discussed having a bit to check if interrupts were pending,
and clear them separately (or whether to make this optional), but
couldn\u2019t think of a case where you\u2019d want to enable IRQs, but not
clear pending interrupts.
- Discussion of enums versus #defines for various things (like SPI
mode). No clear conclusion here. We all agree that enums for error
codes are verboten, and that we should be consistent about usage of
enums or defines.
- Good points for enums: autocompletion in IDEs, debugger shows name vs
value, type safety
- Bad points for enums: unless packed, they take up a full integer.
You need to look up the type of the parameter, which takes up just as
much time as looking up the autocompleted value.
- We\u2019re kinda mixed, with nobody feeling particularly strongly.
Whatever folks think, I think we\u2019d be fine adhering too.
- OS device APIs are not doxygened, they need to be.
- HAL SPI: will/chris will respond with changes discussed here.
they\u2019ve also been looking at HAL SPI recently.
Cheers,
Sterling
Re: Notes from HAL discussions on sterly_refactor
Posted by will sanfilippo <wi...@runtime.io>.
Comments inline
> On Sep 19, 2016, at 6:40 PM, Christopher Collins <cc...@apache.org> wrote:
>
> Hi Will,
>
> On Mon, Sep 19, 2016 at 06:07:55PM -0700, will sanfilippo wrote:
>> The new HAL SPI API is shown below. Highlights of the changes are:
> [...]
>
> These changes look great. I just have a comment and a question:
>
>> /**
>> * Initialize the SPI, given by spi_num.
>> *
>> * @param spi_num The number of the SPI to initialize
>> * @param cfg HW/MCU specific configuration,
>> * passed to the underlying implementation, providing extra
>> * configuration.
>> * @param spi_type SPI type (master or slave)
>> *
>> * @return int 0 on success, non-zero error code on failure.
>> */
>> int hal_spi_init(int spi_num, void *cfg, uint8_t spi_type);
>
> My comment here might apply to the HAL in general. I wonder if there is
> any value in including an init function in the HAL abstraction.
> Initialization is hardware-specific, as evidenced by the void*
> parameter, so wouldn't it be better to just have the caller invoke the
> specific function that applies to the MCU? Unless there is a need to
> initialize a SPI without needing to know what MCU you are running on, I
> would say the init function should be left out of the HAL, as there is
> nothing being abstracted away.
>
Well, you do bring up a point, but I feel there is some benefit in having a consistent hal_xxx_init function across HALs. If only so that developers know they must implement and call this function at startup.
This API does allow for one small abstration though, the spi type (master or slave). Yeah, that is nit-picky :-)
>> /**
>> * Sets the default value transferred by the slave. Not valid for master
>> *
>> * @param spi_num SPI interface to use
>> *
>> * @return int 0 on success, non-zero error code on failure.
>> */
>> int hal_spi_slave_set_def_tx_val(int spi_num, uint16_t val);
>
> Does this function specify the transmitted character until the next
> transceive operation, or does it only apply for txrx operations with a
> null rx buffer? Put another way, if a slave executes the following
> sequence of operations:
>
> 1. Initialize SPI.
> 2. Enable SPI.
> 3. Set default character to 0x99.
>
> And then nothing else, will the slave respond to the master with 0x99
> characters?
>
> Thanks,
> Chris
This description definitely needs improving and I think this API needs a bit of thought as well. To answer your question, yes, that was part of the intent. It was also intended to cover these cases:
1) A NULL txbuf was used as the parameter to hal_spi_txrx_noblock() and the SPI is configured as a slave.
2) The master sends more data than the value supplied in ‘cnt’.
Admittedly, I think this presumes some HW support. It may not be possible to guarantee what happens in these cases. There may be some SPI devices that simply re-transmit the last byte loaded or do something completely different.
Thanks for the comments!
Re: Notes from HAL discussions on sterly_refactor
Posted by Christopher Collins <cc...@apache.org>.
Hi Will,
On Mon, Sep 19, 2016 at 06:07:55PM -0700, will sanfilippo wrote:
> The new HAL SPI API is shown below. Highlights of the changes are:
[...]
These changes look great. I just have a comment and a question:
> /**
> * Initialize the SPI, given by spi_num.
> *
> * @param spi_num The number of the SPI to initialize
> * @param cfg HW/MCU specific configuration,
> * passed to the underlying implementation, providing extra
> * configuration.
> * @param spi_type SPI type (master or slave)
> *
> * @return int 0 on success, non-zero error code on failure.
> */
> int hal_spi_init(int spi_num, void *cfg, uint8_t spi_type);
My comment here might apply to the HAL in general. I wonder if there is
any value in including an init function in the HAL abstraction.
Initialization is hardware-specific, as evidenced by the void*
parameter, so wouldn't it be better to just have the caller invoke the
specific function that applies to the MCU? Unless there is a need to
initialize a SPI without needing to know what MCU you are running on, I
would say the init function should be left out of the HAL, as there is
nothing being abstracted away.
> /**
> * Sets the default value transferred by the slave. Not valid for master
> *
> * @param spi_num SPI interface to use
> *
> * @return int 0 on success, non-zero error code on failure.
> */
> int hal_spi_slave_set_def_tx_val(int spi_num, uint16_t val);
Does this function specify the transmitted character until the next
transceive operation, or does it only apply for txrx operations with a
null rx buffer? Put another way, if a slave executes the following
sequence of operations:
1. Initialize SPI.
2. Enable SPI.
3. Set default character to 0x99.
And then nothing else, will the slave respond to the master with 0x99
characters?
Thanks,
Chris
Re: Notes from HAL discussions on sterly_refactor
Posted by will sanfilippo <wi...@runtime.io>.
The new HAL SPI API is shown below. Highlights of the changes are:
* Moved spi_type, transmit/receive callback and argument out of hal_spi_settings and into device specific structure. This was done as these are the parameters we might expect someone to want to reconfigure an initialized SPI with. They are also generic across SPI implementation.
* There is now a blocking and non-blocking API: hal_spi_txrx() is blocking; hal_spi_txrx_noblock() is the non-blocking interface. This was done as setting the callback to NULL or not NULL to denote blocking/non-blocking was hard to see/understand. Furthermore, you should be able to use the HAL in non-blocking mode (if you want) with a callback configured.
* If you want to configure the SPI using hal_spi_config, the spi must be disabled first (or not yet enabled).
* Made it more clear that if you use greater than 8-bit data words for the SPI, you pass an array of uint16_t values to the API.
* Changed ‘len’ to ‘cnt’ in the hal_spi_txrx and hal_spi_txrx_noblock API so that it was more obvious we are talking about “number of values” as opposed to “bytes"
Comments, as always, appreciated...
/* Prototype for tx/rx callback */
typedef void (*hal_spi_txrx_cb)(void *arg, int len);
/**
* since one spi device can control multiple devices, some configuration
* can be changed on the fly from the hal
*/
struct hal_spi_settings {
uint8_t data_mode;
uint8_t data_order;
uint8_t word_size;
uint32_t baudrate;
};
/**
* Initialize the SPI, given by spi_num.
*
* @param spi_num The number of the SPI to initialize
* @param cfg HW/MCU specific configuration,
* passed to the underlying implementation, providing extra
* configuration.
* @param spi_type SPI type (master or slave)
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_init(int spi_num, void *cfg, uint8_t spi_type);
/**
* Configure the spi. Must be called after the spi is initialized (after
* hal_spi_init is called) and when the spi is disabled (user must call
* hal_spi_disable if the spi has been enabled through hal_spi_enable prior
* to calling this function). Can also be used to reconfigure an initialized
* SPI (assuming it is disabled as described previously).
*
* @param spi_num The number of the SPI to configure.
* @param psettings The settings to configure this SPI with
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_config(int spi_num, struct hal_spi_settings *psettings);
/**
* Sets the txrx callback (executed at interrupt context) when the
* buffer is transferred by the master or the slave using the non-blocking API.
* Cannot be called when the spi is enabled. This callback will also be called
* when chip select is de-asserted on the slave.
*
* NOTE: This callback is only used for the non-blocking interface and must
* be called prior to using the non-blocking API.
*
* @param spi_num SPI interface on which to set callback
* @param txrx Callback function
* @param arg Argument to be passed to callback function
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_set_txrx_cb(int spi_num, hal_spi_txrx_cb txrx_cb, void *arg);
/**
* Enables the SPI. This does not start a transmit or receive operation;
* it is used for power mgmt. Cannot be called when a SPI transfer is in
* progress.
*
* @param spi_num
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_enable(int spi_num);
/**
* Disables the SPI. Used for power mgmt. It will halt any current SPI transfers
* in progress.
*
* @param spi_num
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_disable(int spi_num);
/**
* Blocking call to send a value on the SPI. Returns the value received from the
* SPI slave.
*
* MASTER: Sends the value and returns the received value from the slave.
* SLAVE: Invalid API. Returns 0xFFFF
*
* @param spi_num Spi interface to use
* @param val Value to send
*
* @return uint16_t Value received on SPI interface from slave. Returns 0xFFFF
* if called when the SPI is configured to be a slave
*/
uint16_t hal_spi_tx_val(int spi_num, uint16_t val);
/**
* Blocking interface to send a buffer and store the received values from the
* slave. The transmit and receive buffers are either arrays of 8-bit (uint8_t)
* values or 16-bit values depending on whether the spi is configured for 8 bit
* data or more than 8 bits per value. The 'cnt' parameter is the number of
* 8-bit or 16-bit values. Thus, if 'cnt' is 10, txbuf/rxbuf would point to an
* array of size 10 (in bytes) if the SPI is using 8-bit data; otherwise
* txbuf/rxbuf would point to an array of size 20 bytes (ten, uint16_t values).
*
* NOTE: these buffers are in the native endian-ness of the platform.
*
* 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
*
* @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);
/**
* Non-blocking interface to send a buffer and store received values. Can be
* used for both master and slave SPI types. The user must configure the
* callback (using hal_spi_set_txrx_cb); the txrx callback is executed at
* interrupt context when the buffer is sent.
*
* The transmit and receive buffers are either arrays of 8-bit (uint8_t)
* values or 16-bit values depending on whether the spi is configured for 8 bit
* data or more than 8 bits per value. The 'cnt' parameter is the number of
* 8-bit or 16-bit values. Thus, if 'cnt' is 10, txbuf/rxbuf would point to an
* array of size 10 (in bytes) if the SPI is using 8-bit data; otherwise
* txbuf/rxbuf would point to an array of size 20 bytes (ten, uint16_t values).
*
* NOTE: these buffers are in the native endian-ness of the platform.
*
* 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: Slave "preloads" the data to be sent to the master (values
* stored in txbuf) and places received data from master in rxbuf
* (if not NULL). The txrx callback occurs when len values are
* transferred or master de-asserts chip select. If txbuf is NULL,
* the slave transfers its default byte. Both rxbuf and txbuf cannot
* be NULL.
*
* @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_noblock(int spi_num, void *txbuf, void *rxbuf, int cnt);
/**
* Sets the default value transferred by the slave. Not valid for master
*
* @param spi_num SPI interface to use
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_slave_set_def_tx_val(int spi_num, uint16_t val);
/**
* This aborts the current transfer but keeps the spi enabled.
*
* @param spi_num SPI interface on which transfer should be aborted.
*
* @return int 0 on success, non-zero error code on failure.
*
* NOTE: does not return an error if no transfer was in progress.
*/
int hal_spi_abort(int spi_num);
> On Sep 19, 2016, at 4:07 PM, Sterling Hughes <st...@apache.org> wrote:
>
> Howdy:
>
> We’ve been going over a painstaking review of the various HAL APIs, to make sure we capture all the changes we want to include prior to merge.
>
> Today, we looked at the driver framework, hal GPIO and SPI, here are the changes we discussed:
>
> - GPIO needs to re-name the GPIO state enums to have an hal_* in the name
>
> - Driver functions in adc_dev and elsewhere should be static const to save RAM.
>
> - HAL GPIO: we should remove set/clear/toggle, and just have hal_gpio_write(pin, val)
>
> - HAL GPIO: there should be a function hal_gpio_release() so that ports can be turned off when a pin is no longer used.
> - Q: should hal_gpio_init_in/out follow the more common config(), enable(), disable() paradigm in the rest of the HAL?
>
> - HAL GPIO: hal_gpio_irq_enable() should by default clear any pending IRQs. We discussed having a bit to check if interrupts were pending, and clear them separately (or whether to make this optional), but couldn’t think of a case where you’d want to enable IRQs, but not clear pending interrupts.
>
> - Discussion of enums versus #defines for various things (like SPI mode). No clear conclusion here. We all agree that enums for error codes are verboten, and that we should be consistent about usage of enums or defines.
> - Good points for enums: autocompletion in IDEs, debugger shows name vs value, type safety
> - Bad points for enums: unless packed, they take up a full integer. You need to look up the type of the parameter, which takes up just as much time as looking up the autocompleted value.
> - We’re kinda mixed, with nobody feeling particularly strongly. Whatever folks think, I think we’d be fine adhering too.
>
> - OS device APIs are not doxygened, they need to be.
>
> - HAL SPI: will/chris will respond with changes discussed here. they’ve also been looking at HAL SPI recently.
>
> Cheers,
>
> Sterling
>