FW-299: I2C Implementation #5
Conversation
bzzling
commented
Nov 10, 2024
bzzling
commented
- created i2c.h and i2c.c
- transferred code from FWXV to use HAL library instead
libraries/ms-common/inc/i2c.h
Outdated
| GpioAddress scl; | ||
| } I2CSettings; | ||
|
|
||
| // Initializes selected I2C peripheral |
There was a problem hiding this comment.
For all comments in the header file please follow this format (doxygen format)
/**
* @brief Write a buffer of data into the flash memory
* @details This does not rewrite the flash memory. Once written, the data must be cleared by erasing the entire page
* @param address Memory address to store the buffer. This value MUST be 4-byte aligned
* @param buffer Pointer to the buffer of data to store
* @param buffer_len Length of the buffer to write. This MUST also be 4-byte aligned
* @return STATUS_CODE_OK if flash memory was written succesfully
* STATUS_CODE_OUT_OF_RANGE if address is out of range
* STATUS_CODE_INTERNAL_ERROR if flash write failed
*/
libraries/ms-common/src/arm/i2c.c
Outdated
| // SCL should be open drain as well, but this was not working with hardware | ||
| // Since we only use one master shouldn't be an issue | ||
| gpio_init_pin(&(settings->scl), GPIO_ALTFN_PUSH_PULL, GPIO_STATE_HIGH); | ||
| gpio_init_pin(&(settings->sda), GPIO_ALFTN_OPEN_DRAIN, GPIO_STATE_HIGH); |
There was a problem hiding this comment.
For all gpio_init_pin functions, please use the alternative function
StatusCode gpio_init_pin_af(const GpioAddress *address, const GpioMode pin_mode, GpioAlternateFunctions alt_func);
all I2C will use GPIO_ALT4_I2C1. Remove Mitch's workaround, we will use GPIO_ALFTN_OPEN_DRAIN as the mode
libraries/ms-common/src/arm/i2c.c
Outdated
| s_port[i2c].multi_txn = false; | ||
| res = prv_txn(i2c, addr, rx_data, rx_len, true); | ||
| mutex_unlock(&s_port[i2c].mutex); | ||
| // Return status code ok unless an error has occurred |
libraries/ms-common/src/arm/i2c.c
Outdated
|
|
||
| StatusCode i2c_read_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *rx_data, | ||
| size_t rx_len) { | ||
| if (i2c >= NUM_I2C_PORTS || rx_len > I2C_MAX_NUM_DATA) { |
There was a problem hiding this comment.
For all read/write functions, please check the address is valid. I2C address is 7 bit so it must be <=127
There was a problem hiding this comment.
Also please add an argument check to verify rx_data != NULL
| return res; | ||
| } | ||
|
|
||
| StatusCode i2c_write_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *tx_data, |
There was a problem hiding this comment.
Checks on address and tx_data
libraries/ms-common/src/arm/i2c.c
Outdated
| // Receive data from queue | ||
| // If less than requested is received, an error in the transaction has occurred | ||
| for (size_t rx = 0; rx < len; rx++) { | ||
| if (queue_receive(&s_port[i2c].i2c_buf.queue, &data[rx], 0)) { |
There was a problem hiding this comment.
This is also no longer needed, the HAL function will handling receiving data
| return STATUS_CODE_OK; | ||
| } | ||
|
|
||
| StatusCode i2c_init(I2CPort i2c, const I2CSettings *settings) { |
There was a problem hiding this comment.
Please verify settings != NULL
libraries/ms-common/src/arm/i2c.c
Outdated
| uint32_t periph; // Clock config | ||
| uint8_t ev_irqn; // Event Interrupt | ||
| uint8_t err_irqn; // Error Interrupt | ||
| I2C_TypeDef *base; // Base Peripheral defined by CMSIS |
There was a problem hiding this comment.
This is not defined by CMSIS, it is defined in stm32l433xx.h
libraries/ms-common/src/arm/i2c.c
Outdated
| bool curr_mode; // Mode used in current transxn | ||
| Mutex mutex; // Ensure port only accessed from one task at time | ||
| I2CAddress current_addr; // Addr used in current transxn | ||
| volatile uint8_t num_rx_bytes; // Number of bytes left to receive in rx mode |
There was a problem hiding this comment.
This is not needed. I don't see it used anywhere
libraries/ms-common/src/arm/i2c.c
Outdated
| Mutex mutex; // Ensure port only accessed from one task at time | ||
| I2CAddress current_addr; // Addr used in current transxn | ||
| volatile uint8_t num_rx_bytes; // Number of bytes left to receive in rx mode | ||
| volatile StatusCode exit; // Flag set if error occurs in txn |
There was a problem hiding this comment.
This is not set to anything other than STATUS_CODE_OK. Don't see the point of keeping this
There was a problem hiding this comment.
This code is a good start and gets some of the core ideas, but will need to fundamentally differ from FWXV code. Please read Interrupt mode IO operation at this link. This will give you an understanding of what exactly will be happening. @Akashem06 please verify this logic. I think we should use binary semaphores to wait/lock each bus and have the interrupt handler's HAL_I2C_MasterTxCpltCallback (and Rx version) post the semaphores.
| .err_irqn = I2C2_ER_IRQn }, | ||
| }; | ||
|
|
||
| // Generated using the I2C timing |
There was a problem hiding this comment.
Not sure if this comment should be here
| }; | ||
|
|
||
| // Generated using the I2C timing | ||
| static const uint32_t s_i2c_timing[] = { |
| HAL_I2C_DeInit(&s_i2c_handles[i2c]); | ||
| I2CSettings *settings = &s_port[i2c].settings; | ||
| // Manually clock SCL | ||
| gpio_init_pin_af(&settings->scl, GPIO_OUTPUT_PUSH_PULL, GPIO_ALT4_I2C1); |
There was a problem hiding this comment.
Probably shouldn't be using af here. You're trying to recover from a lockup in which we're going to temporarily forego usage of the peripheral and manually clock the line in hopes of getting the slave to release it. Initialize it as a regular GPIO pin and then manually clock it.
| I2CSettings *settings = &s_port[i2c].settings; | ||
| // Manually clock SCL | ||
| gpio_init_pin_af(&settings->scl, GPIO_OUTPUT_PUSH_PULL, GPIO_ALT4_I2C1); | ||
| for (size_t i = 0; i < 16; i++) { |
There was a problem hiding this comment.
hmm I think we should add a delay here to match the timing of what the bus was configured too. Maybe when hw_timer is merged we can add that
| if (__HAL_I2C_GET_FLAG(&s_i2c_handles[i2c], I2C_FLAG_BUSY) == SET) { | ||
| return STATUS_CODE_UNREACHABLE; | ||
| } | ||
| return STATUS_CODE_OK; |
There was a problem hiding this comment.
You'll need to reinitialize the pins to af when reinitializing the peripheral as well.
| StatusCode res = STATUS_CODE_OK; | ||
| status_ok_or_return(mutex_lock(&s_port[i2c].mutex, I2C_TIMEOUT_MS)); | ||
| s_port[i2c].multi_txn = false; | ||
| res = prv_txn(i2c, addr, rx_data, rx_len, true); |
There was a problem hiding this comment.
So this can probably not be a function call and instead just do the I2C read. This code will likely be much more minimal than what the prv_txn because the HAL should take care of more things for us.
| StatusCode res = STATUS_CODE_OK; | ||
| status_ok_or_return(mutex_lock(&s_port[i2c].mutex, I2C_TIMEOUT_MS)); | ||
| s_port[i2c].multi_txn = false; | ||
| res = prv_txn(i2c, addr, tx_data, tx_len, false); |
| } | ||
| // Lock I2C resource | ||
| StatusCode res = STATUS_CODE_OK; | ||
| status_ok_or_return(mutex_lock(&s_port[i2c].mutex, I2C_TIMEOUT_MS)); |
There was a problem hiding this comment.
Might need to swap to semaphores based on the design I suggested in my final review comment. This is because a mutex does not work in an ISR and will instead become a semaphore that can be posted from the ISR.
| return res; | ||
| } | ||
|
|
||
| StatusCode i2c_read_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *rx_data, |
There was a problem hiding this comment.
This looks generally fine based on the old structure but would need to change for the HAL. We really shouldn't even need to call HAL functions here, just call our write then read functions
| return res; | ||
| } | ||
|
|
||
| StatusCode i2c_write_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *tx_data, |
There was a problem hiding this comment.
same comment as read_reg here
|
Also please ping on discord if you'd like to call about this! It's not an easy task so don't worry if you have questions |
