Implement eeprom writing in eepromCmdFnc #19
Conversation
| //TODO: imeplement this | ||
| printf("eeprom writing not implemented yet\n"); | ||
| printf("experimental eeprom write\n"); | ||
| uint32_t arr[1] = { strtol(argv[4], NULL, 16) }; |
There was a problem hiding this comment.
Use better variable name than "arr."
Also variable does not need to be array since it is only 1 32-bit word. You can pass reference to it when calling eepromWrite.
| printf("eeprom writing not implemented yet\n"); | ||
| printf("experimental eeprom write\n"); | ||
| uint32_t arr[1] = { strtol(argv[4], NULL, 16) }; | ||
| eepromWrite(addr, arr, 1); |
There was a problem hiding this comment.
Since writing to EEPROM could brick a user's controller I'd like to see another user prompt before writing the value (e.g. "You are about to write value 0x%08x to address 0x%08x in EEPROM. This could brick your controller. Are you sure you want to do this [Y/n]?")
|
@returnzork Thanks for the pull request! Please see the comments I've made above. |
|
It might be worth protecting certain regions of the EEPROM? In particular 0x02 and 0x04 contain hardware and software versions respectively. Changing this value can cause the official controller firmware to use different pinmux configurations, which will likely lead to malfunction (and could in theory damage the controller!) |
|
@roblabla I'm mixed on how to handle an implementation of this command. When I made my comments on this pull request my thoughts were that if we warn the user with a prompt every time they try to write to EEPROM, as least we warned them, and if they made a typo they can back out before committing the change. Regarding protecting certain sections of EEPROM, I have two issues with that. First, there is the hubris that we have protected the most important sections. I only have a good feeling for a few portions of EEPROM and if we only protect what we know, we are giving a (little bit of a) false sense of security that the other sections can be changed without issue. Second, there is the inconvenience. If someone has the knowledge, who are we to say they aren't trying to change things for a valid reason (maybe they did the research and know it will be safe to have the controller act like it's on an older rev of HW)? In my mind it comes down to how to best protect the user while not inconveniencing them. We could put up warning every time a user tries to write, but that could get annoying. Or we could implement the function but leave it unbuilt by default. This would require the user to rebuild the code before they were able to change any EEPROM values, but that could also be annoying for those who don't want (or aren't able) to rebuild the firmware. Thoughts? |
|
How about we go the other way around? We have large regions of the EEPROM that we know are not going to cause problems and are easily recoverable (like the jingle data - even if you mess up the stock firmware has some backup jingle data). We could build a whitelist of regions that are "safe" to overwrite, and only show a warning if the writes go outside that region. For starters, writing to [0x74, 0x84] (the jingle index range) and [0x800, 0xc00] (the jingle data range) could be considered safe. |
|
I like that idea. However, maybe it would make more sense to whitelist certain areas by making subcommands (e.g. Thinking worst case though, even with the whitelisting for "safe" regions for raw writes you still could end up with a bricked controller (until you change those regions back to something sane). If a user wrote a bogus index or bad jingle data the firmware could crash on boot (unless the error checking is more robust than I remember...). Maybe we're overthinking this. If someone is knowledgeable enough to be (safely) poking values in EEPROM, they should be able to rebuild the code. A solution without checks or warnings that has to be enabled via rebuild, or leaving it unimplemented, since it's a simple function, would be fine by me. |
Maybe it would be best to change the documentation to state that it is not implemented/enabled because it could brick the controller, instead of stating, as it currently does, that just hasn't been added as a feature yet. Alternatively, maybe a build flag that could enable it from fw_cfg.h ? |
Allows the use of eepromCmdFnc to write to the eeprom.
Writes the 2 digit hex code into eeprom.
Example usage:
eeprom write 8 0x826 72
which would write the value of 114 (hex 72) into eeprom memory location 0x826.
Read back with
eeprom read 8 0x826 1 to see the changed value