EEPROM Directory #192
EEPROM Directory #192
Conversation
There was a problem hiding this comment.
I think that this is a really good advancement in an effort to "platformize" shepherd and our MCU code in general, but there are a few improvements we can make to the API. In regards to driver-level vs application level:
Driver-level:
- Raw I2C transactions with EEPROM
- Function for initializing directory structure via a user-defined struct
- Root EEPROM address (0x00) this is kinda redundant but nice to have to remove magic numbers
- Functions for retrieving indexes given a certain key, possibly vice-versa if it isn't too resource intensive
- Functions for retrieving raw values given a certain key or index that the user can recast upstream
Application-level:
- Directory structure (size and keys)
- Wrapper functions to interpret/cast data from raw API
- In this particular case, fault logging
This is kinda one of the first advancements we are making in a general platform, I think eventually the goal will be to provide a R/W interface for Shepherd on the CAN bus (i.e. send a CAN message to retrieve BMS revision, BMS sends it out; or send a CAN message to read out the most recent faults, BMS sends it out)
| } | ||
| } | ||
|
|
||
| int eepromGetIndex(char *key) |
There was a problem hiding this comment.
We probably want an inverse of this function too just for usability (i.e. given a offset, what key is at that index)
| * | ||
| * @param fault_code | ||
| */ | ||
| void logFault(uint32_t fault_code); |
There was a problem hiding this comment.
I think that a more generic eeprom_read_data(char *key, ssize_t size, void *data) , eeprom_read_data(uint8_t index, ssize_t size, void *data) , eeprom_write_data(char *key, ssize_t size, uint8_t *data) , and eeprom_write_data(uint8_t index, ssize_t size, uint8_t data) might be nice to have for a general API
shepherd_bms/src/eepromdirectory.cpp
Outdated
| uint8_t index = EEPROM.read(eepromGetIndex(const_cast<char*>("FAULTS"))); // TODO will need update with new eeprom driver | ||
|
|
||
| /* if the index is at the end of the partition, wrap around (currently store 5 faults, so max = 5 + offset) */ | ||
| if (index == NUM_EEPROM_FAULTS + eeprom_data[eepromGetIndex(const_cast<char*>("FAULTS"))].offset) |
There was a problem hiding this comment.
This can be simplified with modulus %
shepherd_bms/src/eepromdirectory.cpp
Outdated
| return -1; | ||
| } | ||
|
|
||
| void logFault(uint32_t fault_code) |
There was a problem hiding this comment.
Probably want a getRecentFault function too that returns the most recent fault, or returns an array of the faults in order of most recent to least recent
There was a problem hiding this comment.
im realizing naming and comments were not clear, so ill update those, but this goes for this comment and next: "getRecentFault" function is essentially implemented in updateFaults. updateFaults fills a static, local/volatile array with the current data in eeprom. I weighed using a static array with a function that just updates the data vs a function that returns a pointer to the data, but ended up with this way. So basically the user just needs to access that local array ad itll have the most up to data set of faults as long as "updateFaults" has been called
There was a problem hiding this comment.
i felt like this way creates a certain level of consistency with how we manage bms data, since anaylzer is one set of data that just reupdates with each loop
shepherd_bms/src/eepromdirectory.cpp
Outdated
| EEPROM.put(index, fault_code); // TODO will need update with new eeprom driver | ||
| } | ||
|
|
||
| void updateFaults() |
There was a problem hiding this comment.
I might be missing something but what is the point of this function? Is it to update the most recent fault pointer? Should it be put at the end of logFault?
dyldonahue
changed the title
|
UPDATED CHANGES:
|
This is meant to be a Proof oof concept, not fully built.
This creates an eeprom "directory" with a struct to store all "items", and a function to dynamically initialize eeprom addresses based on these items. I also then used this to create example functionality with faults
main concerns:
I feel like im missing a layer to the directory. The goal of this directory was to generalize eeprom access, but i still ended up having to customize things pretty significantly to fit the example implementation (ie, still had custom fault read/write functions that utilized specific fault nums to work). I assume theres a better way to generalize this further
im going crazy but compiler was giving warnings without const casting the char* because it didnt wanna convert a string to char* so i had to make it messy. I swear i just did the same thing with the fault log and didnt get those warnings so idk
want to avoid making this feel like driver level code, and some of these functions sorta felt like driver level stuff. Not sure if this is a concern or not