Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FW-299: I2C Implementation #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions libraries/ms-common/inc/i2c.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
#pragma once

/************************************************************************************************
* i2c.h
*
* I2C Library Header file
*
* Created: 2024-11-09
* Midnight Sun Team #24 - MSXVI
************************************************************************************************/

/* Standard library headers */
#include <stddef.h>
#include <stdint.h>

/* Inter-component Headers */

/* Intra-component Headers */
#include "gpio.h"
#include "status.h"

/**
* @brief I2C timeout in milliseconds
*/
#define I2C_TIMEOUT_MS 100

/**
* @brief None of our I2C transactions should need to be longer than 32 bytes
*/
#define I2C_MAX_NUM_DATA 32

/**
* @brief I2C address type
*/
typedef uint8_t I2CAddress;

/**
* @brief I2C port type
*/
typedef enum {
I2C_PORT_1 = 0,
I2C_PORT_2,
NUM_I2C_PORTS,
} I2CPort;

/**
* @brief I2C speed type
*/
typedef enum {
I2C_SPEED_STANDARD = 0, // 100kHz
I2C_SPEED_FAST, // 400 kHz
NUM_I2C_SPEEDS,
} I2CSpeed;

/**
* @brief I2C settings struct
*/
typedef struct {
I2CSpeed speed;
GpioAddress sda;
GpioAddress scl;
} I2CSettings;

/**
* @brief Initialize the selected I2C peripheral
*
* @param i2c I2C port to initialize. Must be one of the values from the I2CPort enum.
* @param settings Pointer to the I2CSettings struct.
*
* @return STATUS_CODE_OK if the initialization was successful.
* STATUS_CODE_INVALID_ARGS if given invalid arguments.
* STATUS_CODE_INTERNAL_ERROR if initialization failed.
*/
StatusCode i2c_init(I2CPort i2c, const I2CSettings *settings);

/**
* @brief Reads |rx_len| data from specified address at port
*
* @param i2c The I2C port to use.
* @param addr The I2C address to read from.
* @param rx_data Pointer to where the received data will be stored.
* @param rx_len The number of bytes to read.
*
* @return STATUS_CODE_TIMEOUT if I2C_TIMEOUT_MS is exceeded.
* STATUS_CODE_INTERNAL_ERROR if bus issue has occurred, transaction should be retried.
*/
StatusCode i2c_read(I2CPort i2c, I2CAddress addr, uint8_t *rx_data, size_t rx_len);

/**
* @brief Writes |tx_len| data to specified address at port
*
* @param i2c
* @param addr
* @param tx_data
* @param tx_len
*
* @return STATUS_CODE_TIMEOUT if I2C_MUTEX_WAIT_MS is exceeded.
* STATUS_CODE_INTERNAL_ERROR if bus issue has occurred, transaction should be retried
*/
StatusCode i2c_write(I2CPort i2c, I2CAddress addr, uint8_t *tx_data, size_t tx_len);

/**
* @brief Reads from register address specified by reg.
*/
StatusCode i2c_read_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *rx_data, size_t rx_len);

/**
* @brief Writes to register address specified by reg
*/
StatusCode i2c_write_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *tx_data,
size_t tx_len);

#ifdef MS_PLATFORM_X86

/**
* @brief Used for tests only. Sets given data in the rx queue.
*/
StatusCode i2c_set_data(I2CPort i2c, uint8_t *tx_data, size_t tx_len);

/**
* @brief Used for tests only. Reads data from the tx_queue.
*/
StatusCode i2c_get_data(I2CPort i2c, uint8_t *rx_data, size_t rx_len);
#endif
265 changes: 265 additions & 0 deletions libraries/ms-common/src/arm/i2c.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
/************************************************************************************************
* i2c.c
*
* I2C Library Source Code
*
* Created: 2024-11-09
* Midnight Sun Team #24 - MSXVI
************************************************************************************************/

/* Standard library headers */
#include <stdbool.h>
#include <stdio.h>

/* Inter-component Headers */
#include "stm32l4xx_hal.h"
#include "stm32l4xx_hal_i2c.h"
#include "stm32l4xx_hal_rcc.h"

/* Intra-component Headers */
#include "i2c.h"
#include "interrupts.h"
#include "log.h"
#include "queues.h"
#include "semaphore.h"

typedef enum I2CMode {
I2C_MODE_TRANSMIT = 0,
I2C_MODE_RECEIVE,
NUM_I2C_MODES,
} I2CMode;

// Used to protect I2C resources
// All data tx'd and rx'd through queue
// Transactions on each I2C port protected by Mutex
typedef struct {
uint8_t buf[I2C_MAX_NUM_DATA];
Queue queue;
Semaphore wait_txn;
} I2CBuffer;

typedef struct {
uint32_t periph; // Clock config
uint8_t ev_irqn; // Event Interrupt
uint8_t err_irqn; // Error Interrupt
I2C_TypeDef *base; // Base Peripheral defined by STM32L433
I2CSettings settings; // I2C Config
I2CBuffer i2c_buf; // RTOS data structures
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
bool multi_txn; // if we are doing more than one txn don't send stop
} I2CPortData;

static I2CPortData s_port[NUM_I2C_PORTS] = {
[I2C_PORT_1] = { .periph = RCC_APB1ENR1_I2C1EN,
.base = I2C1,
.ev_irqn = I2C1_EV_IRQn,
.err_irqn = I2C1_ER_IRQn },
[I2C_PORT_2] = { .periph = RCC_APB1ENR1_I2C2EN,
.base = I2C2,
.ev_irqn = I2C2_EV_IRQn,
.err_irqn = I2C2_ER_IRQn },
};

// Generated using the I2C timing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this comment should be here

static const uint32_t s_i2c_timing[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to s_i2c_speeds

[I2C_SPEED_STANDARD] = 100000, // 100 kHz
[I2C_SPEED_FAST] = 400000, // 400 kHz
};

static I2C_HandleTypeDef s_i2c_handles[NUM_I2C_PORTS];

// Attempt to recover from a bus lockup
static StatusCode prv_recover_lockup(I2CPort i2c) {
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (size_t i = 0; i < 16; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

gpio_toggle_state(&settings->scl);
}
gpio_init_pin_af(&settings->scl, GPIO_ALTFN_PUSH_PULL, GPIO_ALT4_I2C1);
// Clear I2C error flags
__HAL_I2C_CLEAR_FLAG(&s_i2c_handles[i2c],
I2C_FLAG_BERR | I2C_FLAG_ARLO | I2C_FLAG_OVR | I2C_FLAG_TIMEOUT);
// Disable the I2C peripheral
__HAL_I2C_DISABLE(&s_i2c_handles[i2c]);
// Enable the I2C peripheral
__HAL_I2C_ENABLE(&s_i2c_handles[i2c]);
// Reinitialize
s_i2c_handles[i2c].Instance = s_port[i2c].base;
s_i2c_handles[i2c].Init.OwnAddress1 = 0;
s_i2c_handles[i2c].Init.AddressingMode = I2C_ADDRESSINGMODE_7BIT;
s_i2c_handles[i2c].Init.DualAddressMode = I2C_DUALADDRESS_DISABLE;
s_i2c_handles[i2c].Init.OwnAddress2 = 0;
s_i2c_handles[i2c].Init.GeneralCallMode = I2C_GENERALCALL_DISABLE;
s_i2c_handles[i2c].Init.NoStretchMode = I2C_NOSTRETCH_DISABLE;
if (HAL_I2C_Init(&s_i2c_handles[i2c]) != HAL_OK) {
return STATUS_CODE_INTERNAL_ERROR;
}
if (__HAL_I2C_GET_FLAG(&s_i2c_handles[i2c], I2C_FLAG_BUSY) == SET) {
return STATUS_CODE_UNREACHABLE;
}
return STATUS_CODE_OK;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to reinitialize the pins to af when reinitializing the peripheral as well.

}

StatusCode i2c_init(I2CPort i2c, const I2CSettings *settings) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please verify settings != NULL

if (i2c >= NUM_I2C_PORTS || settings->speed >= NUM_I2C_SPEEDS || settings == NULL) {
return STATUS_CODE_INVALID_ARGS;
}

s_port[i2c].settings = *settings;

// Enable clock for I2C
switch (s_port[i2c].periph) {
case RCC_APB1ENR1_I2C1EN:
__HAL_RCC_I2C1_CLK_ENABLE();
break;
case RCC_APB1ENR1_I2C2EN:
__HAL_RCC_I2C2_CLK_ENABLE();
break;
default:
break;
}

// Enable clock for GPIOB
__HAL_RCC_GPIOB_CLK_ENABLE();

// Initialize pins to correct mode to operate I2C (open drain)
// 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_af(&(settings->scl), GPIO_ALTFN_PUSH_PULL, GPIO_ALT4_I2C1);
gpio_init_pin_af(&(settings->sda), GPIO_ALFTN_OPEN_DRAIN, GPIO_ALT4_I2C1);

s_i2c_handles[i2c].Instance = s_port[i2c].base;
s_i2c_handles[i2c].Init.OwnAddress1 = 0;
s_i2c_handles[i2c].Init.AddressingMode = I2C_ADDRESSINGMODE_7BIT;
s_i2c_handles[i2c].Init.DualAddressMode = I2C_DUALADDRESS_DISABLE;
s_i2c_handles[i2c].Init.OwnAddress2 = 0;
s_i2c_handles[i2c].Init.GeneralCallMode = I2C_GENERALCALL_DISABLE;
s_i2c_handles[i2c].Init.GeneralCallMode = I2C_NOSTRETCH_DISABLE;

if (HAL_I2C_Init(&s_i2c_handles[i2c]) != HAL_OK) {
return STATUS_CODE_INTERNAL_ERROR;
}

interrupt_nvic_enable(s_port[i2c].ev_irqn, INTERRUPT_PRIORITY_NORMAL);
interrupt_nvic_enable(s_port[i2c].err_irqn, INTERRUPT_PRIORITY_NORMAL);
__HAL_I2C_DISABLE_IT(&s_i2c_handles[i2c], I2C_IT_ERRI | I2C_IT_TCI | I2C_IT_STOPI | I2C_IT_NACKI |
I2C_IT_ADDRI | I2C_IT_RXI | I2C_IT_TXI);

// Configure data structures
s_port[i2c].i2c_buf.queue.num_items = I2C_MAX_NUM_DATA;
s_port[i2c].i2c_buf.queue.item_size = sizeof(uint8_t);
s_port[i2c].i2c_buf.queue.storage_buf = s_port[i2c].i2c_buf.buf;
s_port[i2c].multi_txn = false;
status_ok_or_return(sem_init(&s_port[i2c].i2c_buf.wait_txn, 1, 0));
status_ok_or_return(mutex_init(&s_port[i2c].mutex));
status_ok_or_return(queue_init(&s_port[i2c].i2c_buf.queue));

// Enable I2C peripheral
__HAL_I2C_ENABLE(&s_i2c_handles[i2c]);

return STATUS_CODE_OK;
}

// Do a transaction on the I2C bus
static StatusCode prv_txn(I2CPort i2c, I2CAddress addr, uint8_t *data, size_t len, bool read) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the purpose of this function on our old repo was to handle the underlying transaction, and this function was to be called from the public tx/rx functions. This can probably be axed now. Read the rest of my comments to understand why

// Check that bus is not busy - If it is, assume that lockup has occurred
StatusCode status = STATUS_CODE_OK;
if (!s_port[i2c].multi_txn && __HAL_I2C_GET_FLAG(&s_i2c_handles[i2c], I2C_FLAG_BUSY) == SET) {
status_ok_or_return(prv_recover_lockup(i2c));
}
queue_reset(&s_port[i2c].i2c_buf.queue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lock mutex before performing any operations

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm I guess this is taken care of in the upper function. Please leave a comment if that is the case

s_port[i2c].current_addr = (addr) << 1;

// Set up txn for read or write
if (read) {
s_port[i2c].curr_mode = I2C_MODE_RECEIVE;
} else {
s_port[i2c].curr_mode = I2C_MODE_TRANSMIT;
for (size_t tx = 0; tx < len; tx++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filling up the queue is no longer needed

if (queue_send(&s_port[i2c].i2c_buf.queue, &data[tx], 0)) {
return STATUS_CODE_RESOURCE_EXHAUSTED;
}
}
}

// Enable Interrupts before setting start to begin txn
HAL_I2C_Master_Transmit_IT(&s_i2c_handles[i2c], addr, data, len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we transmitting, receiving and enabling listening all at once? This should be dependent on if it is a read/write request

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what Aryan is saying, just a call to HAL_I2C_Master_Transmit_IT should be sufficient. This includes getting rid of the receive as well as the queue as the HAL now handles the buffering for us.

HAL_I2C_Master_Receive_IT(&s_i2c_handles[i2c], addr, data, len);
HAL_I2C_EnableListen_IT(&s_i2c_handles[i2c]);
// Wait for signal that txn is finished from ISR, then disable IT and generate stop
status = sem_wait(&s_port[i2c].i2c_buf.wait_txn, I2C_TIMEOUT_MS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This semaphore is never posted?

HAL_I2C_DisableListen_IT(&s_i2c_handles[i2c]);
if (!s_port[i2c].multi_txn ||
status) { // In an multi exchange or failed txn, need to keep line live to receive data
HAL_I2C_Master_Abort_IT(&s_i2c_handles[i2c], addr);
}
return STATUS_CODE_OK;
}

StatusCode i2c_read(I2CPort i2c, I2CAddress addr, uint8_t *rx_data, size_t rx_len) {
if (i2c >= NUM_I2C_PORTS || rx_len > I2C_MAX_NUM_DATA || rx_data == NULL || addr > 127) {
return STATUS_CODE_INVALID_ARGS;
}
// Lock I2C resource
StatusCode res = STATUS_CODE_OK;
status_ok_or_return(mutex_lock(&s_port[i2c].mutex, I2C_TIMEOUT_MS));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

s_port[i2c].multi_txn = false;
res = prv_txn(i2c, addr, rx_data, rx_len, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mutex_unlock(&s_port[i2c].mutex);
return res;
}

// Address needs to be just the device address, read/write bit is taken care of in hardware
StatusCode i2c_write(I2CPort i2c, I2CAddress addr, uint8_t *tx_data, size_t tx_len) {
if (i2c >= NUM_I2C_PORTS || tx_len > I2C_MAX_NUM_DATA || tx_data == NULL || addr > 127) {
return STATUS_CODE_INVALID_ARGS;
}
// Lock I2C resource
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as read

mutex_unlock(&s_port[i2c].mutex);
return res;
}

StatusCode i2c_read_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *rx_data,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

size_t rx_len) {
if (i2c >= NUM_I2C_PORTS || rx_len > I2C_MAX_NUM_DATA || rx_data == NULL || addr > 127) {
return STATUS_CODE_INVALID_ARGS;
}

StatusCode res = STATUS_CODE_OK;
status_ok_or_return(mutex_lock(&s_port[i2c].mutex, I2C_TIMEOUT_MS));
uint8_t reg_to_read = reg;
s_port[i2c].multi_txn = true;
// Write register value, then read result in second txn
res = prv_txn(i2c, addr, &reg_to_read, sizeof(reg_to_read), false);
if (res) {
mutex_unlock(&s_port[i2c].mutex);
return res;
}
res = prv_txn(i2c, addr, rx_data, rx_len, true);
mutex_unlock(&s_port[i2c].mutex);
return res;
}

StatusCode i2c_write_reg(I2CPort i2c, I2CAddress addr, uint8_t reg, uint8_t *tx_data,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks on address and tx_data

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as read_reg here

size_t tx_len) {
if (i2c >= NUM_I2C_PORTS || tx_len > I2C_MAX_NUM_DATA - 1 || tx_data == NULL || addr > 127) {
return STATUS_CODE_INVALID_ARGS;
}

uint8_t write_data[I2C_MAX_NUM_DATA] = { 0 };
write_data[0] = reg;
for (size_t i = 1; i < tx_len + 1; i++) {
write_data[i] = tx_data[i - 1];
}

status_ok_or_return(i2c_write(i2c, addr, write_data, tx_len + 1));
return STATUS_CODE_OK;
}
Loading