Open citorva opened 1 year ago
The API is pretty clear, condensed and thought through. Thanks for your effort!
Two suggestions:
Thank you for your answer.
I have updated the first message regarding the use of the word "slave".
About the non-blocking API, I don't understand how to make it working.
I thought about callback use called on hardware interrupt, however, I think that the main thread must be blocked during the peripheral lifetime.
However, I can be anyway useful to implement a low level API like that before the API as described above to reduce the platform-specific implementation.
Maybe a non-blocking API could look a bit like this:
/**
* @brief I2C bus initialization parameters for use in peripheral mode
*/
typedef struct {
/* global bus settings come here: speed, address width support, ... */
} i2c_peripheral_params_t;
/**
* @brief Handler specification for an I2C peripheral endpoint
*/
typedef struct {
/**
* @brief Callback function to be called when data was received (written
* to the peripheral by the controller)
* @param addr The address of the peripheral data was received for
* @param data The received data
* @param data_len Length of @p data in bytes
*
* @warning The "ownership" of the buffer @p data points to is returned to
* the I2C driver when this function returns. The I2C driver is
* free to reuse this buffer once this function returns.
*/
void (*rx_callback)(uint16_t addr, const void *data, size_t data_len);
/**
* @brief Callback function to be called when data needs to be send
* (read from the peripheral by the controller)
* @param buf Buffer to write the data in for transmission
* @param size The size of the buffer (cannot send more than this)
* @return The number of bytes written to the buffer
*
* @note This function is only called when no data to transmit was
* previously queued via @ref i2c_periphal_queue_tx_data
* @warning The I2C driver will do clock stretching until this functions
* returns. Not every I2C device supports clock stretching and
* not every I2C controller handles clock stretching correctly.
* If the I2C driver does not support clock stretching,
* registering a driver with `tx_callback != NULL` will fail.
* @note Prefer sending data to the controller via
* @ref i2c_periphal_queue_tx_data if possible
*/
size_t (*tx_callback)(void *buf, size_t buf_size);
} i2c_peripheral_handler_t;
/**
* @brief Initialize I2C hardware for use in peripheral mode (sometimes
* referred to as "slave" mode)
* @param dev Device to initialize
* @param params Parameters to initialize the device with
*
* @retval 0 Success
* @retval <0 Failure with negative errno code to indicate the cause
*/
int i2c_peripheral_init(i2c_t dev, const i2c_peripheral_params_t *params);
/**
* @brief Register an address and corresponding handler specification
* @param dev Device to register the address and handler to
* @param buf The buffer to use for data to send to the controller
* @param buf_len Size of @p buf in bytes
* @param addr I2C peripheral address to respond to
* @param handler Callback functions to call when the controllers
* sending data / requesting data
*
* @warning Not every I2C driver / hardware supports clock stretching. In case
* it doesn't, this function will return `-ENOTSUP` if the TX callback
* in @p handler is not `NULL`.
* @retval 0 Success
* @retval -ENOTSUP Parameters not supported
* @retval -EBUSY All "address slots" are in use, cannot register more
* addresses
* @retval <0 Failure with negative errno code ti indicate the cause
*/
int i2c_peripheral_add_addr(i2c_t dev, void *buf, size_t buf_len, uint16_t addr,
const i2c_peripheral_handler_t *handler);
/**
* @brief Unregister an address
*
* @post When the controller is subsequently trying to communicate with
* that address, this I2C device no longer responds to it
* @post The buffer previously registered will no longer be used by the
* I2C device
*/
void i2c_peripheral_del_addr(i2c_t dev, uint16_t addr);
/**
* @name Flags to pass to @ref i2c_peripheral_queue_tx_data
* @{
*/
/**
* @brief Append data to the end of the TX queue, rather than
* overwriting the queue
*/
#define I2C_PERIPHERAL_QUEUE_FLAG_APPEND 0x01
/**
* @brief Replace data in the TX queue with the new data (if any), rather
* then appending it
*/
#define I2C_PERIPHERAL_QUEUE_FLAG_REPLACE 0x00
/** @} */
/**
* @brief Queue data to transmit to the controller (controller reads from
* peripheral)
* @param dev The I2C device to queue data for sending to
* @param addr The I2C address to send the data from
* @param data The data to send
* @param data_len Length of the data in @p data in bytes
* @param flags Specify exact behavior such as whether the data should
* be appended, or overwrite data in the TX queue.
*
* The I2C driver must send the queued data to the controller when the
* controller next reads from I2C address @p addr. This takes preference over
* the function in @ref i2c_peripheral_handler_t::tx_callback. If the
* controller reads more data than is queued, a NACK will be send rather
* than falling back to calling the TX callback.
*
* @note If feasible, the I2C driver will reply with the queued data without
* clock stretching
*
* @retval 0 Success
* @retval -EOVERFLOW The TX buffer is overflown
*/
int i2c_periphal_queue_tx_data(i2c_t dev, uint16_t addr,
const void *data, size_t data_len,
bool flags);
I tried to optimize the suggested API for:
i2c_peripheral_add_addr()
can then just failPlease keep in mind that the suggested API is not implemented and tested. There may be some really rough corners and bugs in it.
Using i2c_peripheral_add_addr
will require a way to manages registered addresses and, in many devices which enable complex addressing, I fear that it will require dynamic allocation.
I don't know if Riot OS avoid dynamic allocation, but I prefer to avoid it.
I have made this one from your proposition:
/**
* @brief Type of the callback function to be called when data was received
* (written to the peripheral by the controller)
* @param addr The address of the peripheral data was received for
* @param data The received data
* @param data_size The size of the received @p data in bytes
*
* @warning The "ownership" of the buffer @p data points to is returned to the
* I2C driver when this function returns. The I2C driver is free to
* reuse this buffer once this function returns
*/
typedef void (*i2c_peripheral_rx_callback_t)(uint16_t addr,
const void *data,
size_t data_size);
/**
* @brief Type of the callback function to be called when data needs to be
* send (read from the peripheral by the controller)
* @param addr The address of the peripheral data needs to be sent for
* @param buffer The buffer to write the data in the transmission
* @param buffer_size The size of @p buffer in bytes. The data needs to fit
* into the buffer, thus its size needs to be lower than
* the buffer size
* @return The number of bytes written to the buffer
*
* @note This function is only called when no data to transmit was previously
* queued via @ref i2c_peripheral_queue_tx_data
* @warning The I2C driver will do clock stretching until this functions
* returns. Not every I2C device supports clock stretching and not
* every I2C controller handles clock stretching correctly. If the I2C
* driver does not support clock stretching, registering this callback
* function will fail
* @note Prefer sending data to the controller via
* @ref i2c_periphal_queue_tx_data if possible
*/
typedef size_t (*i2c_peripheral_tx_callback_t)(uint16_t addr, void *buffer,
size_t buffer_size);
/**
* @brief Make the peripheral accept all messages to the specified address
* @param dev The I2C device to set address for
* @param address The address listened to by the peripheral
*
* @note Only one address mode must be specified for each I2C peripherals,
* calling @ref i2c_peripheral_set_address,
* @ref i2c_peripheral_set_address_range,
* @ref i2c_peripheral_set_address_with_mask or
* @ref i2c_peripheral_set_address_duet will overwrite the previous
* configuration
*/
void i2c_peripheral_set_address(i2c_t dev, uint16_t address);
/**
* @brief Make the peripheral accept all messages to an address in the given
* range
* @param dev The I2C device to set address for
* @param begin_address The first address in the range
* @param end_address The last address in the range
*
* @note Only one address mode must be specified for each I2C peripherals,
* calling @ref i2c_peripheral_set_address,
* @ref i2c_peripheral_set_address_range,
* @ref i2c_peripheral_set_address_with_mask or
* @ref i2c_peripheral_set_address_duet will overwrite the previous
* configuration
* @warning This method is not supported on all devices which enable I2C in the
* peripheral mode. This function will fail if you attempt to call it
* on an unsupported hardware
*/
void i2c_peripheral_set_address_range(i2c_t dev, uint16_t begin_address,
uint16_t end_address);
/**
* @brief Make the peripheral accept all messages to an address matching with
* the masked one
* @param dev The I2C device to set address for
* @param address The address to match
* @param mask The applied mask
*
* @note Only one address mode must be specified for each I2C peripherals,
* calling @ref i2c_peripheral_set_address,
* @ref i2c_peripheral_set_address_range,
* @ref i2c_peripheral_set_address_with_mask or
* @ref i2c_peripheral_set_address_duet will overwrite the previous
* configuration
* @warning This method is not supported on all devices which enable I2C in the
* peripheral mode. This function will fail if you attempt to call it
* on an unsupported hardware
*/
void i2c_peripheral_set_address_with_mask(i2c_t dev, uint16_t address,
uint16_t mask);
/**
* @brief Make the peripheral accept all messages to one of the two specified
* addresses
* @param dev The I2C device to set address for
* @param address1 The first address to match
* @param address2 The second address to match
*
* @note Only one address mode must be specified for each I2C peripherals,
* calling @ref i2c_peripheral_set_address,
* @ref i2c_peripheral_set_address_range,
* @ref i2c_peripheral_set_address_with_mask or
* @ref i2c_peripheral_set_address_duet will overwrite the previous
* configuration
* @warning This method is not supported on all devices which enable I2C in the
* peripheral mode. This function will fail if you attempt to call it
* on an unsupported hardware
*/
void i2c_peripheral_set_address_duet(i2c_t dev, uint16_t address1,
uint16_t address2);
/**
* @brief Register the callback function called when the peripheral receives
* data from the controller
* @param dev The I2C device to set the callback for
* @param cb The callback function to set
*/
void i2c_peripheral_register_rx_callback(i2c_t dev,
i2c_peripheral_rx_callback_t cb);
/**
* @brief Register the callback function called when the controller claims
* data from the peripheral when no data has not been queued with
* @ref i2c_peripheral_queue_tx_data
* @param dev The I2C device to set the callback for
* @param cb The callback function to set
*
* @warning The I2C driver will do clock stretching until this functions
* returns. Not every I2C device supports clock stretching and not
* every I2C controller handles clock stretching correctly. If the I2C
* driver does not support clock stretching, registering this callback
* function will fail
*/
void i2c_peripheral_register_tx_callback(i2c_t dev,
i2c_peripheral_tx_callback_t cb);
/**
* @brief Queue the data to send to the controller
* @param dev The I2C device to queue the data for
* @param addr The I2C address to queue data for
* @param data The data to queue
* @param data_size The size of the data to queue
*
* @warning The "ownership" of the buffer @p data must be kept after the end of
* this function call and maybe the end of the parent function
* @note Please consider using a static data or ensure that the data will not
* be freed before its transmission by using
* @ref i2c_peripheral_is_data_queued
* @warning Calling this function multiple times with different addresses with
* the same device will not stack the queued data. The last queued data
* pointer will be overwritten by the new queued data
*/
void i2c_peripheral_queue_tx_data(i2c_t dev, uint16_t addr,
const void *data, size_t data_size);
/**
* @brief Check whether the data is still queued
* @param dev The I2C device where check the data is still queued
* @param data The pointer to the possibly queued data
* @return 1 when the data is still queued or 0 else
*/
int i2c_peripheral_is_data_queued(i2c_t dev, const void *data);
Hi, thanks for the reply.
void i2c_peripheral_set_address_duet(i2c_t dev, uint16_t address1,
uint16_t address2);
void i2c_peripheral_set_address_with_mask(i2c_t dev, uint16_t address,
uint16_t mask);
void i2c_peripheral_set_address_range(i2c_t dev, uint16_t begin_address,
uint16_t end_address);
These three essentially try to scratch the same itch. The most likely outcome is that users will end up using the most convenient (set_address_range()
) while drivers have to implement all three. IMO, it is better to only provide one interface for drivers to implement. If that interface is not user-friendly, it would be possible to add a convenience function on top (hence, only a single implementation) and greatly reduce the complexity of the per hardware drivers.
void i2c_peripheral_register_rx_callback(i2c_t dev,
i2c_peripheral_rx_callback_t cb);
I assume that this is expect to be called exactly once? If so, it is IMO better to not use function pointers at all but rather expect the user to implement a function with a given name. With that, there is no state to manage (the callback function is always "there") and direct function calls are more efficient.
But note that compared to the registration of an address in combination with a callback function, a lot of flexibility is lost. If the API would indeed allow to register a callback function along with a corresponding I2C address, it would be relatively trivial to implement e.g. an i2c_peripheral_saul_temperature
module and i2c_peripheral_saul_humidty
module that would set up their handler function and address without the need for any coordination between those two. If there only is a single callback function, this would have to dispatch the handling to the correct callback function of the modules.
Note that no dynamic memory management is required to implement a callback API. A statically allocated array for the registration state would be fully sufficient. If the hardware doesn't limit the number of addresses anyway, something like
#ifndef CONFIG_I2C_MAX_PERIPHERAL_ADDRESS
#define CONFIG_I2C_MAX_PERIPHERAL_ADDRESS 4
#endif
would be sufficient.
An alternative would be to directly pass an array of addresses and corresponding handler functions during driver initialization. By using XFA to populate that array, again no coordination would be needed across modules implementing I2C peripheral interfaces. And the application developer would not be required to count I2C peripheral modules to provide an CONFIG_I2C_MAX_PERIPHERAL_ADDRESS
. Anther advantage would be that error handling (e.g. when more I2C peripheral addresses are needed than the hardware supports) would only be needed in once place, rather than in every module implementing an I2C peripheral interface.
I don't know what XFA means in this context. By rapidly searching the term on the internet, I find Xml Form Architecture, which I don't think that it is. Can you explain XFA?
XFA stands for cross file arrays, see https://doc.riot-os.org/xfa_8h.html#details for the documentation of it.
It basically allows each C file to add elements to a array. The linker will then collect all array elements, sort them alphabetically, and place them in contiguous memory. At runtime you can query the length of an XFA with XFA_LEN()
.
In practice, it could look like this:
typdef struct {
void (*rx_callback)(uint16_t addr, const void *data, size_t data_len, void *ctx);
size_t (*tx_callback)(void *buf, size_t buf_size, void *ctx);
void *ctx;
uint16_t addr;
} i2c_peripheral_handler_t;
and in a module implementing a peripheral I2C interface:
static void i2c_temp_rx_callback(uint16_t addr, const void *data, size_t data_len, void *ctx)
{
/* prepare sensor values for tx and queue them ... */
}
/* extend statically allocated array of I2C peripheral interfaces */
XFA(xfa_i2c_peripheral_handlers, 0) i2c_peripheral_handler_t _xfa_i2c_peripheral_handler_temperature = {
.rx_callback = i2c_temp_rx_callback,
.ctx = temperature_sensor_handle,
.addr = 0x42
};
And in periph_init
the driver would be initialized using:
XFA_USE(i2c_peripheral_handler_t, xfa_i2c_peripheral_handlers);
i2c_peripheral_init(I2C_DEV(0), xfa_i2c_peripheral_handlers, XFA_LEN(xfa_i2c_peripheral_handlers));
Note that above is not full picture, as it would only work with one I2C bus (but at least in principle this should allow presenting I2C peripheral on multiple I2C busses). But I hope it is concrete enough to explain the idea.
Sounds interesting. However, because it appears to add complexity, I think that it is also important to handle XFA in the common code. Consequently, I think that I need to define two levels of the API:
Foreword
I had to use the I2C in the peripheral mode in a project using Riot OS in order to make several cards communicating to each other in a bus topology.
However, this mode was impossible to realize with the current Riot OS API because the current I2C driver does not support I2C in the peripheral mode.
My aim in this document is to describe a draft API in order to implement the peripheral mode in the operating system.
The API design principles
The strength of Riot OS is its architecture built around the use of a scheduler, making possible the multithreading.
With this feature, the choice has been made to rely on this operation in order to offer blocking functions in the high-level API, preventing the development of the interruption routine by the end user.
I am actually inspired by the Atmel's SERCOM peripheral in the addressing mode.
To-do
Example
Here is an example using the API in a basic application
Sample header
Here is the sample API of the project: