Feature/eeprom struct #56
Conversation
|
This is some major cleanup! Good work! |
There was a problem hiding this comment.
great to see this PR!
I've only done a very quick first pass check in this review (I am running off to uni in a few mins, so low on time)
needs some more careful review. Main Q is if we try to make this strictly a structure change for this PR, then follow-up separately with logic changes
Src/signal.c
Outdated
| @@ -14,6 +14,7 @@ | |||
| #include "sounds.h" | |||
| #include "targets.h" | |||
|
|
|||
| EEprom_t eepromBuffer; | |||
There was a problem hiding this comment.
why is this in signal.c? would main.c make more sense?
Src/sounds.c
Outdated
| @@ -322,18 +322,18 @@ void playBeaconTune3() | |||
| // fwdgt_counter_reload(); | |||
| // signaltimeout = 0; | |||
|
|
|||
| // if(eepromBuffer[i] == 255){ | |||
| // if(eepromBuffer.buffer[i] == 255){ | |||
There was a problem hiding this comment.
as i has changed, this commented out code is now incorrect. Maybe remove the commented code? or fix it to use the tune[] array?
| @@ -2,8 +2,8 @@ | |||
| ; *** Scatter-Loading Description File generated by uVision *** | |||
| ; ************************************************************* | |||
|
|
|||
| LR_IROM1 0x08001000 0x0000FFFF { ; load region size_region | |||
| ER_IROM1 0x08001000 0x0000FFFF { ; load address = execution address | |||
| LR_IROM1 0x08001000 0x00007BDF { ; load region size_region | |||
There was a problem hiding this comment.
any reason why this is changing? does this impact the keil build?
There was a problem hiding this comment.
its just "more correct" because thats the hard limit for the region
Src/main.c
Outdated
| if (eepromBuffer.eeprom_version > 2) { | ||
| if (eepromBuffer.auto_advance > 1) { | ||
| eepromBuffer.auto_advance = 0; | ||
| save_flash_nolib(eepromBuffer.buffer, sizeof(eepromBuffer.buffer), eeprom_address); |
There was a problem hiding this comment.
why re-save here? each save is a risk. If we lose power during a save the user can end up with incorrect settings.
|
needs a rebase |
freasy
force-pushed
the
feature/eeprom_struct
branch
2 times, most recently
from
c16b8ea
to
7238301
Compare
# Conflicts: # Src/main.c
freasy
force-pushed
the
feature/eeprom_struct
branch
from
7238301
to
701f7f6
Compare
removed the uint8_t array and replaced with a struct
removed buffer variables, and refactored with direct eeprom struct access