Skip to content

Conversation

@nordicjm
Copy link
Contributor

@nordicjm nordicjm commented Mar 23, 2022

PR for adding a device information mcumgr group. Functionality is similar to the linux uname command where a format string can be provided to return certain information including board name, processor type, architecture, kernel name/version, OS name. It can be extended by other code using the revamped mcumgr callback system to add additional output/processing or to replace the OS name.

The device information handler can be used to retrieve information about
the configuration of the configured device such as board name, board
revision, firmware version and build date.

Example output, no parameters:
Zephyr
'a' parameter, with default Bluetooth enabled settings:
Zephyr Zephyr zephyr-v3.0.0-2569-g7f0654f969f7 3.0.99 arm unknown bt510 Zephyr

  • Base implementation
  • Build date/time support (with note about invalidating reproducible builds, disabled by default)
  • Tests (check right response is received, check if memory error is received if buffer size is too small, check overriding OS name)
  • Release notes update
  • Documentation

This requires #50578 as a base and has pulled those commits into this review (first 7 commits of this PR can be ignored)

@nordicjm nordicjm requested a review from de-nordic March 23, 2022 13:26
Copy link
Contributor

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

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

I think that this can be addition to OS group, there is no need to create additional group for that.
Consider also making the command more complex, like for example BSD uname, where request could be filled with optional "format" string that would be assigned letters representing requested info, for example

format="bfa";

would give response:

b="<CONFIG_BOARD>"
f="0.1.2-something"
unsupported="a"

@nordicjm
Copy link
Contributor Author

@de-nordic Moved to OS and uname-style output, still some parts to implement. Thoughts on this? Not sure if the processor response is already available or if it would become an #ifdef mess with many types of processors and processor families. Is there a way to currently get the overall package that you know of e.g. "Zephyr" for if purely using zephyr or "nRF connect SDK" if using that, etc.?

@de-nordic
Copy link
Contributor

@de-nordic Moved to OS and uname-style output, still some parts to implement. Thoughts on this? Not sure if the processor response is already available or if it would become an #ifdef mess with many types of processors and processor families. Is there a way to currently get the overall package that you know of e.g. "Zephyr" for if purely using zephyr or "nRF connect SDK" if using that, etc.?

I do not think that we have a way to get processor name for now.
During compilation we do not know processor name only the arch, maybe processor name could be pulled out of DTS somehow.
We can probably postpone the processor part and complete all other "letters".

@nordicjm nordicjm force-pushed the dev_mcumgr1 branch 2 times, most recently from 6d1a6f7 to c916d9c Compare April 12, 2022 08:26
@nordicjm
Copy link
Contributor Author

Added a custom function to allow additional options in application, and changed OS to be it's own (weak) function to allow overriding by vendors

@nordicjm nordicjm requested a review from de-nordic April 19, 2022 10:07
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 19, 2022
@github-actions github-actions bot closed this Jul 3, 2022
@de-nordic
Copy link
Contributor

@nordicjm Should we bring this back to live?

@nordicjm
Copy link
Contributor Author

Would be a good feature to add, yes.

@de-nordic
Copy link
Contributor

Would be a good feature to add, yes.

Can we re-open this and set target to 3.3?

@nordicjm nordicjm reopened this Sep 23, 2022
@nordicjm nordicjm added this to the v3.3.0 milestone Sep 23, 2022
@github-actions github-actions bot removed the Stale label Sep 24, 2022
@nordicjm nordicjm force-pushed the dev_mcumgr1 branch 3 times, most recently from 3d7b5ee to a52f352 Compare September 27, 2022 08:15
@nordicjm nordicjm force-pushed the dev_mcumgr1 branch 3 times, most recently from 36b0294 to 6030d2c Compare October 31, 2022 11:57
@nordicjm nordicjm force-pushed the dev_mcumgr1 branch 2 times, most recently from 4e77442 to 1759bdf Compare November 3, 2022 12:52
@nordicjm nordicjm requested a review from de-nordic November 3, 2022 12:53
@nordicjm
Copy link
Contributor Author

nordicjm commented Nov 3, 2022

Rebased

@nordicjm nordicjm force-pushed the dev_mcumgr1 branch 3 times, most recently from 230035a to efe0905 Compare November 10, 2022 12:41
@nordicjm nordicjm force-pushed the dev_mcumgr1 branch 3 times, most recently from 94c106f to 6567930 Compare November 18, 2022 11:35
Copy link
Contributor

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

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

One comment regarding Kconfig names.

@nordicjm
Copy link
Contributor Author

nordicjm commented Dec 5, 2022

@de-nordic Changes made, left some of the text as-is to prevent duplicating strings

@nordicjm nordicjm requested a review from de-nordic December 5, 2022 11:16
The device information handler can be used to retrieve information about
the configuration of the configured device such as board name, board
revision, firmware version and build date.

Signed-off-by: Jamie McCrae <[email protected]>
Adds details on the OS information mcumgr command.

Signed-off-by: Jamie McCrae <[email protected]>
Adds details of a new os_mgmt command for getting OS/application
information.

Signed-off-by: Jamie McCrae <[email protected]>
Adds tests for the os_mgmt info command.

Signed-off-by: Jamie McCrae <[email protected]>
Fixes an issue with large packets being received, these packets are
chunked into 127-byte frames for the serial transport but this system
is not needed for the dummy transport as it has a fixed size buffer.

Signed-off-by: Jamie McCrae <[email protected]>
Copy link
Contributor

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

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

I do not like the (prior_output == true ... things in sprintf as I think that saving on strings will not be so significant with branching overhead.
Having said that I do not find it a deal breaker, as this is something we can argue on later and improve (or not).

Looks OK, Thanks.

@nordicjm
Copy link
Contributor Author

nordicjm commented Dec 7, 2022

@fabiobaltieri can you merge this please?

@fabiobaltieri
Copy link
Member

@fabiobaltieri can you merge this please?

Needs a second review, I'll take a look.

* Will be unknown if processor type is not listed
* (List extracted from /cmake/gcc-m-cpu.cmake)
*/
#if defined(CONFIG_ARM)
Copy link
Member

Choose a reason for hiding this comment

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

Wow this is really hard to follow, maybe worth indenting the defines in a followup patch? Not normally a fan of that either but editing this seems like a nightmare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is horrible to look at and update, indentation would probably be a good idea, I'll give it a try and see if CI throws an error about it

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if it's even worth reporting it in such details, maybe you could just report whatever is in cpu@0 compatible.

@fabiobaltieri fabiobaltieri merged commit 5d8c79f into zephyrproject-rtos:main Dec 7, 2022
@nordicjm nordicjm deleted the dev_mcumgr1 branch January 6, 2023 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Documentation area: mcumgr Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants