Repository navigation
Dproano/logging - #2
jibbychohan wants to merge 44 commits into
Conversation
0fcc76d to
2be47d1
Compare
7cb22ae to
74f8652
Compare
|
|
||
| flash_error_t write_to_cache(uint32_t col_addr, uint8_t *data, size_t len); | ||
|
|
||
| flash_error_t program(uint32_t addr); |
There was a problem hiding this comment.
Naming this parameter "addr" is misleading because I'm pretty sure it's really a row address. Need to confirm during testing because the datasheet isn't very specific.
There was a problem hiding this comment.
same with the other functions; all the function that deal with a specific page deal in row addr
74a004c to
bdab5f7
Compare
bbe0d92 to
2f3395e
Compare
jacobh460
left a comment
jacobh460
left a comment
There was a problem hiding this comment.
I think it's looking pretty good just the biggest thing is the clock prescaler looks wrong now (I agree that you probably had it correct originally). And some things in the logging.cpp. I'll add more comments later if I think of something else.
|
Can you also document in one of the files or in a markdown how you are structuring the bad page table in memory and how the logging logic works (ie it looks like you are making sure a structure doesn't cross page boundary) |
1ea3c34 to
2d5b9cb
Compare
jacobh460
left a comment
jacobh460
left a comment
There was a problem hiding this comment.
I think it's looking pretty good, just some documentation and a few small things and I think it's good to test and then merge. Also we should make sure DFU won't overwrite the BPT in flash. And if you could make sure to comment the functions in bad_page_table.h/.cpp to make it clear what they are doing to other people.
| constexpr uint32_t BPT_MAX_ENTRIES = | ||
| (FLASH_SECTOR_SIZE - STM32_FLASH_WORD_SIZE - NUM_BYTES_TRACKING_BLOCKS) / STM32_FLASH_WORD_SIZE; | ||
|
|
||
| const uint32_t *bpt_magic = &_bpt_start; |
There was a problem hiding this comment.
These give the impression that the data pointed to is constant; what I meant is can you make it clear that these point to fixed addresses.
There was a problem hiding this comment.
The syntax for this is uint32_t* const bpt_magic = ... which makes it clear that the address pointed to doesn't change, but the data at that address can be modified/
| return !(bpt_table[get_bpt_bad_byte(block_num)] & get_bpt_bad_bit(block_num)); | ||
| } | ||
|
|
||
| bool get_runtime_bad_page(uint32_t block_num, uint32_t *out_page) { |
There was a problem hiding this comment.
This function looks like it's checking if a block was marked bad at runtime and returns the last good page that was written to? Can you just comment that this is what it does and what the return value means thx
| return *out_page != NAND_NUM_PAGES; | ||
| } | ||
|
|
||
| bool add_entry(uint32_t page_address) { |
There was a problem hiding this comment.
Same with this - can you make a note that it stores which specific page went bad.
| return *reinterpret_cast<uint32_t *>(get_entry_addr(index)); | ||
| } | ||
|
|
||
| uint32_t get_bpt_bad_byte(uint32_t block_num) { |
There was a problem hiding this comment.
Consider renaming to get_bpt_bad_index or get_bpt_bad_byte_index
| return block_num / 8; | ||
| } | ||
|
|
||
| uint8_t get_bpt_bad_bit(uint32_t block_num) { |
There was a problem hiding this comment.
Consider renaming to get_bpt_bad_bit_mask
| } >FLASH | ||
|
|
||
| /* Reserved for the NAND flash Bad Page Table */ | ||
| .bpt_table (NOLOAD) : |
There was a problem hiding this comment.
Can you confirm DFU won't overwrite this section
| static bool in_flight = false; | ||
|
|
||
| static void dump_log_cmd() { | ||
| if (!in_flight && Logging::dump()) { | ||
| CommsSerial.println("Log dump finished"); | ||
| } | ||
| } | ||
|
|
||
| static void erase_log_cmd() { | ||
| if (!in_flight) { | ||
| Logging::erase(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you move these (the dump_log_cmd and erase_log_cmd) to logging.cpp; and see my other comment about the log_arm logic, it's similar to what you are going for here. You can call CommandRouter::add() from Logging::begin() to keep things organized. We usually try to call CommandRouter::add() from a lib's dedicated begin() function to keep the ec_main.cpp organized. The only exception to this is when we have a lib that is shared across multiple different devices (ie also used by the prog mcu), in which case we may put some logic in main.
| -D USBCON | ||
| -D USBD_USE_CDC |
There was a problem hiding this comment.
these two are redundant since they are already specified on lines 34-35
No description provided.