Skip to content
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
4 changes: 2 additions & 2 deletions bms/App/api/eeprom/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ add_library(eeprom STATIC)

file(GLOB SRC_FILES "*.c")
target_sources(eeprom PRIVATE ${SRC_FILES})

target_include_directories(eeprom PUBLIC .)
target_link_libraries(eeprom PUBLIC stm32_hal)
target_include_directories(eeprom PUBLIC .)
135 changes: 135 additions & 0 deletions bms/App/api/eeprom/eeprom.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
#include "eeprom.h"

#define EEPROM_ADDRESS 0x50

void setWriteProtect(int enable) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i would make these separate functions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in firmware we want to use fixed width integers. so int should be uint8_t or uint16_t or int32_t etc

if (enable) {
HAL_GPIO_WritePin(WriteProtect_EEPROM_GPIO_Port, WriteProtect_EEPROM_Pin, GPIO_PIN_SET);
} else {
HAL_GPIO_WritePin(WriteProtect_EEPROM_GPIO_Port, WriteProtect_EEPROM_Pin, GPIO_PIN_RESET);
}
}

int eeprom_init()
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why does this function need a return type?

setWriteProtect(1);
return 0;
}

int eeprom_ready()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if this function is limited to core/ only, i suggest taking the HAL return types and handling them.

{
HAL_StatusTypeDef status = HAL_I2C_IsDeviceReady(&hi2c1, (EEPROM_ADDRESS << 1), 3, 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the 3 and 1 here are called magic numbers, which you should avoid.

try to find a way to make them passed as variables or at the very least named

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same thing with EEPROM_ADDRESS. why isnt it <<1'd to begin with? (rhetorical)


if (status == HAL_OK)
{
return 1;
}
else if (status == HAL_BUSY || status == HAL_TIMEOUT)
{
return 0;
}

return -1;
}

int eeprom_wait_until_ready(uint32_t timeout_ms)
{
uint32_t tickstart = HAL_GetTick();
while ((HAL_GetTick() - tickstart) < timeout_ms)
{
int ready = eeprom_ready();
if (ready == 1) return 0;
else if (ready == -1) return -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

errors need to be handled more granularly. -1 is not enough information, and when you test code, you will definitely want more info

HAL_Delay(5);
}
return -1;
}

int eeprom_write(uint16_t address, uint8_t *data, size_t size)
{
if (size > 128)
return -1;

setWriteProtect(0);

HAL_StatusTypeDef status = HAL_I2C_Mem_Write(
&hi2c1,
(EEPROM_ADDRESS << 1),
address,
I2C_MEMADD_SIZE_16BIT,
data,
size,
10);

setWriteProtect(1);

if (status != HAL_OK)
return -1;

return 0;
}

int eeprom_write_dma(uint16_t address, uint8_t *data, size_t size) {
if (hi2c1.State != HAL_I2C_STATE_READY) return -1;

setWriteProtect(0);

HAL_StatusTypeDef status = HAL_I2C_Mem_Write_DMA(
&hi2c1,
(EEPROM_ADDRESS << 1),
address,
I2C_MEMADD_SIZE_16BIT,
data,
size
);

if (status != HAL_OK) {
setWriteProtect(1);
return -1;
}

return 0;
}

void eeprom_TxCpltCallback() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the idea is there, but call back functions must match the function signature as called by the IRQ.

in this instance, you should use HAL_I2C_MemTxCpltCallback or whichever is applicable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

eeprom_TxCpltCallback will never get called by the IRQ

setWriteProtect(1);
}

int eeprom_read(uint16_t address, uint8_t *buffer, size_t size)
{
if (buffer == NULL || size == 0)
return -1;

HAL_StatusTypeDef status = HAL_I2C_Mem_Read(
&hi2c1,
(EEPROM_ADDRESS << 1),
address,
I2C_MEMADD_SIZE_16BIT,
buffer,
size,
10);

if (status != HAL_OK)
return -1;

return 0;
}

int eeprom_read_dma(uint16_t address, uint8_t *buffer, size_t size) {
if (hi2c1.State != HAL_I2C_STATE_READY) return -1;

HAL_StatusTypeDef status = HAL_I2C_Mem_Read_DMA(
&hi2c1,
(EEPROM_ADDRESS << 1),
address,
I2C_MEMADD_SIZE_16BIT,
buffer,
size
);

if (status != HAL_OK) {
return -1;
}

return 0;
}
18 changes: 18 additions & 0 deletions bms/App/api/eeprom/eeprom.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#pragma once

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

while this is correct and works, we use standard include guards across all project modules, so this should be updated accordingly.

#include "stm32g4xx_hal.h"
#include "main.h"

extern I2C_HandleTypeDef hi2c1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good use of extern


int eeprom_init();

int eeprom_ready();
int eeprom_wait_until_ready(uint32_t timeout_ms);

int eeprom_write(uint16_t address, uint8_t* data, size_t size);
int eeprom_read(uint16_t address, uint8_t* buffer, size_t size);

int eeprom_write_dma(uint16_t address, uint8_t* data, size_t size);
int eeprom_read_dma(uint16_t address, uint8_t* buffer, size_t size);

void eeprom_TxCpltCallback();
1 change: 1 addition & 0 deletions bms/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ target_link_libraries(${PROJECT_NAME}
adbms
segment
charger
eeprom
program
daemons
threads
Expand Down
2 changes: 2 additions & 0 deletions bms/Core/Inc/main.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ void Error_Handler(void);
#define T_SWDIO_GPIO_Port GPIOA
#define T_SWCLK_Pin GPIO_PIN_14
#define T_SWCLK_GPIO_Port GPIOA
#define WriteProtect_EEPROM_Pin GPIO_PIN_11

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i would use your own copy in side eeprom.h and not use these incase codegen messes up one day

#define WriteProtect_EEPROM_GPIO_Port GPIOC

/* USER CODE BEGIN Private defines */

Expand Down
2 changes: 2 additions & 0 deletions bms/Core/Inc/stm32g4xx_it.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ void UsageFault_Handler(void);
void DebugMon_Handler(void);
void DMA1_Channel1_IRQHandler(void);
void DMA1_Channel2_IRQHandler(void);
void DMA1_Channel3_IRQHandler(void);
void DMA1_Channel4_IRQHandler(void);
void FDCAN1_IT0_IRQHandler(void);
void EXTI15_10_IRQHandler(void);
void TIM5_IRQHandler(void);
Expand Down
Loading
Loading