Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flash metadata #308

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add flash metadata #308

wants to merge 15 commits into from

Conversation

kepler452b123
Copy link
Contributor

@kepler452b123 kepler452b123 commented Nov 3, 2024

Purpose

Read below
Link to notion task: https://www.notion.so/uworbital/Modify-flash-layout-to-store-app-metadata-b4c9aab740cc4f65be1b6852e9623190?pvs=4

New Changes

  • Storing metadata by directly writing to flash during download process of BL
  • Created linker symbol to store origin of metadata
  • Created source files to read from metadata
  • Documented steps to add more metadata
  • Re-added changes to C code from Asynchronous logging PR to use the correct ports for Rev1 for BL

Testing

-With BL, and app sent over UART using the metadata test example:
Screenshot 2024-11-03 014225
-Without BL, app used over uniflash using the metadata test example
Screenshot 2024-11-03 015219
-Erase test (flashing 2 binaries using BL consecutively), the two bin sizes should be different
Screenshot 2024-11-03 015949
Screenshot 2024-11-03 021033

Outstanding Changes

  • Could add a CUSTOM_END_ADDRESS option to remove any bytes taken from APP_FLASH when ENABLE_BL_BYPASS is on
  • Add metadata_test to examples (update according to Yarik's cmake PR)
  • Could add a python generation script that can generate metadata.c for us (makes it more scalable than copying the bytes for each struct property we add)
    -Unneeded currently IMO unless we plan on storing more than checksum + some cmake definitions later on
  • Add ccmake definition -DFLASH_SERIAL so that if we call metadata.c functions then it does not seg fault if -DENABLE_BL_BYPASS is 0 and we try to flash both the app binary and bl using uniflash instead of using serial

NOTE: Flashing the BL and then the app binary using uniflash (instead of sending the app binary through serial) will cause the readAppMetadata function to segfault if DENABLE_BL_BYPASS is set to false. This is because the memory addresses will not have anything in them, however since DENABLE_BL_BYPASS will be set to 0, the function will not return no metadata err code and instead try to access the memory

Copy link

github-actions bot commented Nov 3, 2024

Pull reviewers stats

Stats of the last 120 days for UWOrbital:

User Total reviews Time to review Total comments
Yarik-Popov 29 1d 16h 51m 270
kepler452b123 12 9h 35m 71
Navtajh04 9 11d 6h 7m 55
dgobalak 3 7h 51m 3

⚡️ Pull request stats

#define MAX_FNAME_LINENUM_SIZE 128U
#define MAX_FNAME_LINENUM_SIZE 150U
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure where this is from

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was from one of Natvaj's logging prs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was the logging pr merged? if so, try pulling from main

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldve been merged

@Yarik-Popov Yarik-Popov changed the title Kashifb/flash metadata Add flash metadata Nov 3, 2024
Copy link
Contributor

@Yarik-Popov Yarik-Popov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some else needs to review this as well.

obc/app/metadata/metadata.h Outdated Show resolved Hide resolved
obc/app/metadata/metadata.c Outdated Show resolved Hide resolved
obc/app/metadata/metadata.h Outdated Show resolved Hide resolved
obc/tools/python/bin_formatter.py Outdated Show resolved Hide resolved
obc/app/metadata/metadata.h Outdated Show resolved Hide resolved
obc/app/sys/obc_errors.h Outdated Show resolved Hide resolved
#define MAX_FNAME_LINENUM_SIZE 128U
#define MAX_FNAME_LINENUM_SIZE 150U
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was from one of Natvaj's logging prs

obc/bl/bl_main.c Outdated Show resolved Hide resolved
obc/bl/bl_main.c Outdated Show resolved Hide resolved
obc/bl/bl_main.c Outdated Show resolved Hide resolved
int32_t blUartWriteBufferLen =
snprintf(blUartWriteBuffer, BL_MAX_MSG_SIZE, "Failed to init flash, error code: %d\r\n", errCode);
if (blUartWriteBufferLen < 0) {
blUartWriteBytes(BL_UART_SCIREG, strlen("Error with processing message buffer length\r\n"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bl error logging should be identical to obc err logging (LOG_ERR_CODE(x)), but ill let @ThenujaL handle this in his PR since hes already porting the logging stuff over

obc/bl/bl_main.c Outdated Show resolved Hide resolved
obc/bl/bl_main.c Outdated

blFlashFapiInitBank(0U);

errCode = blFlashFapiBlockErase(METADATA_START_ADDRESS, METADATA_SIZE_BYTES - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why - 1? I dont think we do that anywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metadata is from 13fff8 to 13ffff (8). currently the erase function deletes the entire sector of the address. 13fff8 + 8 = 140000, so without the -1 it would delete the entire sector starting at 140000 (undefined behavior)

#include <stdint.h>

typedef enum {
BL_UART_SCIREG_1 = 0,
BL_UART_SCIREG_2 = 1,
BL_UART_SCIREG = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why this enum needs to exist anymore since we only have (and will ever have?) 1 value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea i just added it from the old asynch logging PR. i can remove it

obc/app/metadata/metadata.c Outdated Show resolved Hide resolved
#define MAX_FNAME_LINENUM_SIZE 128U
#define MAX_FNAME_LINENUM_SIZE 150U
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was the logging pr merged? if so, try pulling from main

obc/app/sys/obc_errors.h Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change this? this layer supports both uart ports. why change it to only support 1?

Copy link
Contributor Author

@kepler452b123 kepler452b123 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the synchronous logging PR implemented this change, it was reverted but we found that the BL didn't work properly on the Rev1 without the port changes from that PR. i could use #if directive to check if board type is OBC_REVISION_1 nd then only use the one port from rev1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants