Conversation
wxkim
left a comment
wxkim
left a comment
There was a problem hiding this comment.
good work. i have left file specific comments below.
some overall feedback:
your error handling is a good start, but is a little weak right now. eventually, logging at the api level will simply call log(data), and you will need to handle everything under the hood, including errors, automatically moving the write address, and keeping track of where the last write/read operation was under power loss, etc
in order for your code to support that, you will need to use objects to keep track of everything, so i recommend integrating your errors and states into enums and structs to hold all of that.
also make sure to use fixed width integers (and specify signed or unsigned).
keep in mind that you will eventually have to start thinking about how to format data upon log being called. you will have to design your code around EEPROM read and write lifecycles as well
|
|
||
| #define EEPROM_ADDRESS 0x50 | ||
|
|
||
| void setWriteProtect(int enable) { |
There was a problem hiding this comment.
i would make these separate functions
| return 0; | ||
| } | ||
|
|
||
| void eeprom_TxCpltCallback() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
eeprom_TxCpltCallback will never get called by the IRQ
|
|
||
| #define EEPROM_ADDRESS 0x50 | ||
|
|
||
| void setWriteProtect(int enable) { |
There was a problem hiding this comment.
in firmware we want to use fixed width integers. so int should be uint8_t or uint16_t or int32_t etc
| } | ||
|
|
||
| int eeprom_init() | ||
| { |
There was a problem hiding this comment.
why does this function need a return type?
|
|
||
| int eeprom_ready() | ||
| { | ||
| HAL_StatusTypeDef status = HAL_I2C_IsDeviceReady(&hi2c1, (EEPROM_ADDRESS << 1), 3, 1); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
same thing with EEPROM_ADDRESS. why isnt it <<1'd to begin with? (rhetorical)
| return 0; | ||
| } | ||
|
|
||
| int eeprom_ready() |
There was a problem hiding this comment.
if this function is limited to core/ only, i suggest taking the HAL return types and handling them.
| { | ||
| int ready = eeprom_ready(); | ||
| if (ready == 1) return 0; | ||
| else if (ready == -1) return -1; |
There was a problem hiding this comment.
errors need to be handled more granularly. -1 is not enough information, and when you test code, you will definitely want more info
| @@ -0,0 +1,18 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
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; |
| #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 |
There was a problem hiding this comment.
i would use your own copy in side eeprom.h and not use these incase codegen messes up one day
wxkim
added
enhancement
3 participants
Implements basic read and write functions with DMA and blocking modes and uses write protect pin. Untested but it builds. I'm not sure exactly what data is being stored or how it should be structured on the eeprom, so i haven't done anything for that.
closes #131
closes #132