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

boot_serial: minor fixes in 'erase_range()' log output and for 'bs_list()' #1803

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pepe2k
Copy link
Contributor

@pepe2k pepe2k commented Sep 8, 2023

This short series includes 3 minor fixes for boot_serial:

  1. drop redundant parameters from bs_list()
    The buf and len parameters aren't used inside bs_list() function.

  2. boot_serial: include flash area offset in log info
    Currently, log info in erase_range() and bs_upload() shows range being erased/written relative to selected flash area which might be misleading. Include flash area offset in output so that the absolute addresses of the range being erased/written are shown.

  3. don't use %j length modifier with Zephyr's minimal libc
    The Zephyr's 'Minimal C library' doesn't support the %j length modifier when cbprintf is configured with minimum features.

The 'buf' and 'len' parameters aren't used inside 'bs_list()' function.

Signed-off-by: Piotr Dymacz <pepe2k@gmail.com>
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Seems like the "Writing at ..." parts should be updated also

@pepe2k
Copy link
Contributor Author

pepe2k commented Sep 19, 2023

@nordicjm

Seems like the "Writing at ..." parts should be updated also

I've added that in second commit (boot_serial: include flash area offset in log info).

Thanks!

@nordicjm
Copy link
Collaborator

Format needs updating for CI to pass

@@ -596,8 +596,13 @@ static off_t erase_range(const struct flash_area *fap, off_t start, off_t end)
}

size = flash_sector_get_off(&sect) + flash_sector_get_size(&sect) - start;
BOOT_LOG_INF("Erasing range 0x%jx:0x%jx", (intmax_t)start,
(intmax_t)(start + size - 1));
#if defined(__ZEPHYR__) && defined(CONFIG_MINIMAL_LIBC) && defined(CONFIG_CBPRINTF_NANO)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No. Avoid Zephyr specific CONFIG_ things in code.
You can define two different formatting strings on top of the unit and guard them with CONIFG.
Probably using inttypes.h definitions for PRI would be helpful here.
Maybe we need to change the string to avoid using intmax at all if possible, at all the pointer size should be known prior to compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@de-nordic

No. Avoid Zephyr specific CONFIG_ things in code.

OK. Let me think about alternative solution.

@@ -240,7 +240,7 @@ bs_list_img_ver(char *dst, int maxlen, struct image_version *ver)
* List images.
*/
static void
bs_list(char *buf, int len)
bs_list(void)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are conditionally compiled in calls, like bs_set, that still take both parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@de-nordic yes, but the bs_set() makes use of these parameters, the bs_list() doesn't. Am I not seeing something obvious here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, sorry. I have looked at diffs quickly and missed that the bs_set is called from different function.

Currently, log info in 'erase_range()' and 'bs_upload()' shows range
being erased/written relative to selected flash area which might be
misleading. Include flash area offset in output so that the absolute
addresses of the range being erased/written are shown.

Signed-off-by: Piotr Dymacz <pepe2k@gmail.com>
The Zephyr's 'Minimal C library' doesn't support the '%j' length
modifier when 'cbprintf' is configured with minimum features.

Signed-off-by: Piotr Dymacz <pepe2k@gmail.com>
@pepe2k pepe2k marked this pull request as draft December 7, 2023 09:37
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.

None yet

3 participants