sys/app_metadata: Add a way to print application metadata by MrKevinWeiss · Pull Request #10961 · RIOT-OS/RIOT

MrKevinWeiss · 2019-02-06T12:30:21Z

Contribution description

This PR adds a command to print a number of useful application parameters such as BOARD, MCU, OS_VERSION, APP_NAME, etc.
The print function prints in json as it is human readable (enough) and machine readable/easy to format.
It also adds an optional parameter APP_SHELL_FMT (application shell format) intended to help coordinate any specific formatting that the application is following. It can be specified in the makefile or in commandline but if not specified it doesn't print anything.

Testing procedure

run BOARD=<???> make flash term -C tests/shell/ or APP_SHELL_FMT="test" BOARD=<???> make flash term -C tests/shell/ or add APP_SHELL_FMT="test" in the makefile.
Then in the shell check the response when calling app_metadata
Or just call make flash test -C tests/shell/

Issues/PRs references

None

miri64 · 2019-02-06T13:05:18Z

Meta data on what?

miri64 · 2019-02-06T13:07:45Z

Meta data on what?

After looking at the code: how about renaming the module app_metadata to make this clearer.

kaspar030 · 2019-02-06T13:08:51Z

sys/Makefile.include

 endif

+ifneq (,$(filter metadata,$(USEMODULE)))
+  # Overwrite the interface version


what interface is this referring to? API?

None right now, the plan it to have the RDM merged then we can specify it adheres to the rules. Similar to something like it adheres to a JSON schema draft 7.

What RDM? There are 3 under review right now?

None right now, the plan it to have the RDM merged then we can specify it adheres to the rules. Similar to something like it adheres to a JSON schema draft 7.

Though I like the simplicity of this implementation, I wonder

a) Why pin-point to JSON, when we also support e.g. CBOR?
b) Why use printf and not fmt if and only if we want to stick to JSON.

The RDM I am referring to is #10624

kaspar030 · 2019-02-06T13:09:36Z

sys/include/metadata.h

+/**
+ * @brief Prints information about metadata to stdio.
+ */
+void metadata_get(void);


"get" implies this goes somewhere. maybe "print" would be more appropriate?

Yup, that what I had originally but saw something with get... I will change it!

MrKevinWeiss · 2019-02-06T13:13:13Z

@miri64 sure thing, I wasn't that happy with the naming anyways. app_metadata it will be.

kaspar030 · 2019-02-06T13:15:10Z

I must ask for the use of this. Having the "metadata" printed is quite useless unless for the shell.

How about containing this in sys/shell/commands?

I'd name it "about". Everyone seeing a prompt will type "help", and everyone will try "about".

MrKevinWeiss · 2019-02-06T13:17:57Z

@kaspar030 Ok, is "about" fine with you @miri64

miri64 · 2019-02-06T13:22:00Z

@kaspar030 Ok, is "about" fine with you @miri64

Don't care much about that :-)

miri64 · 2019-02-06T13:22:46Z

Don't care much about that :-)

I mean the name of the shell command. The module should still be named app_metadata or something like that.

miri64 · 2019-02-06T13:23:05Z

about as a module name would be too unspecific.

kaspar030 · 2019-02-06T13:27:18Z

about as a module name would be too unspecific.

that's why I suggest to just put the code into sys/shell/commands/sc_about.c. Not even a module needed.

miri64 · 2019-02-06T13:30:49Z

that's why I suggest to just put the code into sys/shell/commands/sc_about.c. Not even a module needed.

I prefer to always have a corresponding module to a shell command. This way we have a way to later integrate it into other configuration/information mechanisms (e.g. via CoAP).

MichelRottleuthner · 2019-02-06T13:41:59Z

Actually I would have done it like @kaspar030 wrote above, i.e. just adding a sc_about.c. But thinking about it, I agree that putting it to a separate module is more flexible.
Another thing: The dedicated "test" can probably be removed as it doesn't really add something. tests/shell should be enough, no?

MrKevinWeiss · 2019-02-06T13:48:26Z

Also isn't the USEMODULE the way to select which shell commands are used (I tried to model this after the ps command)

kaspar030 · 2019-02-06T13:49:41Z

I just realized that the module outputs in json format...

Look, my first reaction was as @miri64's: "what metadata?"

I know how this works. You're happily solving some problem (in this case, maybe you've been working on the unified testing RDM) and need some functionality. It has something to do with metadata, and 15min later there's a PR for "sys/metadata".

This unfortunately blanks out that there's a plethora of different understandings for metadata. The PR description is not very helpful (it could mention json format, and that it is supposed to be used by the tests, ...). So as is, there's "metadata" in "sys/", but noone gets what the folder is there for.

I suspect there are gonna be more related functions. So how about directly creating a "sys/test" module? IMO, it is the right place for a function to print test metadata. It could be the in-tree code support for the coming RDM.
Soon there'll be "test_print_result_value()", "test_print_success()", ... Surely we don't want "sys/result_value" as module.

miri64 · 2019-02-06T13:49:47Z

Also isn't the USEMODULE the way to select which shell commands are used (I tried to model this after the ps command)

Yepp, but in theory you could just define a pseudo-module for that (e.g. like I did to provide the pktbuf command -- which is still implemented in gnrc_pktbuf_static, but usually not exposed to the shell per default).

MrKevinWeiss · 2019-02-06T14:52:02Z

@kaspar030 Heh, correct you got me 😁 and I will improve the description of the PR. I added this because I thought it would be useful regardless to have this from an application perspective. Yes the formatting is json but it is also somewhat human readable which has been the standard for most of this.

@MichelRottleuthner the test was more to show how to use it. It can also be used just for testing and dropped before merge (I think @cladmi is doing something like that). As you wish.

In terms of what is metadata, you see the information that I am wanting. Just a mechanism for the application to identify itself, I am really not tied to the name and appreciate anyone input (or simply say do it like this)

I can also move it to sys/tests but I am more thinking we should have some consensus. For me it seems like there is more than one way to skin a cat but maybe we can hit two birds with one stone... :)

@miri64 Thanks for the pseudo-module hint!

miri64 · 2019-02-06T14:55:03Z

In terms of what is metadata, you see the information that I am wanting. Just a mechanism for the application to identify itself, I am really not tied to the name and appreciate anyone input (or simply say do it like this)

Yes, we can see that after looking at the code. But the name of the module, the description of the shell command, and the descriptive text of the PR should also make that clear, so people don't assume "Oh I need metadata for feature X, and there is a metadata module. It is probably supposed to go there?!??"

MrKevinWeiss · 2019-02-06T14:58:42Z

@miri64 Yup, bad name, got it. Any suggestions? The "about" command also seems reasonable. Maybe "app_info"?

miri64 · 2019-02-06T15:00:37Z

@miri64 Yup, bad name, got it. Any suggestions? The "about" command also seems reasonable. Maybe "app_info"?

app_metadata is fine with me. Just metadata was a little bit confusing to me.

MrKevinWeiss · 2019-02-06T15:01:48Z

I am fine with that as well!

miri64 · 2019-02-06T15:02:41Z

Also, please find a more specific name than INTERFACE_VERSION or interface. It's already hard enough for people to find out if others talk about network interfaces or APIs [edit: or whatever else exists in the world of interfaces], so calling a specific API just "the interface" is very confusing.

MrKevinWeiss · 2019-02-06T15:08:20Z

Yup yup. Just need to think of what is specific enough!

This allows the application code to know what application it is running

This allows a access to application metadata such as BOARD, CPU, etc. It prints the contents to the stdio in a standard json form

This make an easily accessable shell command to print app_metadata

MichelRottleuthner

Reading over the comments, requests and changes again I think besides the one minor thing stated below everything is resolved by now and its ready to be merged - @kaspar030 , @miri64 you agree?.

MichelRottleuthner · 2019-03-06T08:33:02Z

sys/Makefile.include

 endif

+ifneq (,$(filter app_metadata,$(USEMODULE)))
+  # Overwrite the interface version


The comment # Overwrite the interface version is still using the very unspecific wording interface version. How about something more explicit about the fact that this modifies the reported version of the standardized shell output format? (and to explain the context maybe a reference to the RDM?)

Adds app_metadata to the shell test This both serves as a simple test to see if the module is available and doesn't crash as well as an example

MrKevinWeiss · 2019-03-06T12:34:43Z

😗 Thanks for the review @MichelRottleuthner

miri64 · 2019-03-07T10:00:43Z

@kaspar030 , @miri64 you agree?.

For reference: yepp, all comments were addressed :-)

MrKevinWeiss self-assigned this Feb 6, 2019

MrKevinWeiss requested review from MichelRottleuthner, cladmi and smlng February 6, 2019 12:30

kaspar030 reviewed Feb 6, 2019

View reviewed changes

MrKevinWeiss added State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 6, 2019

MrKevinWeiss changed the title ~~sys/metadata: Add a metadata command to let an application give metadata~~ sys/app_metadata: Add a way to print application metadata Feb 7, 2019

MrKevinWeiss removed the State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. label Feb 7, 2019

jcarrano mentioned this pull request Feb 11, 2019

[TRACKING] The RIOT shell as a testing interface #10994

Open

9 tasks

MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 12, 2019

Makefile: add APPLICATION to macro defines

0738084

This allows the application code to know what application it is running

MrKevinWeiss force-pushed the pr/addshellcommands branch from eede925 to 5f9ea3f Compare March 5, 2019 13:23

MrKevinWeiss added 2 commits March 5, 2019 16:10

sys/app_metadata: Add app_metadata module

fc80ae7

This allows a access to application metadata such as BOARD, CPU, etc. It prints the contents to the stdio in a standard json form

sys/shell: Add shell command for app_metadata

7a220d1

This make an easily accessable shell command to print app_metadata

MrKevinWeiss force-pushed the pr/addshellcommands branch 2 times, most recently from a3a032a to e635799 Compare March 5, 2019 16:54

MichelRottleuthner requested changes Mar 6, 2019

View reviewed changes

tests/shell: Add app_metadata to shell tests

c36cf57

Adds app_metadata to the shell test This both serves as a simple test to see if the module is available and doesn't crash as well as an example

MrKevinWeiss force-pushed the pr/addshellcommands branch from e635799 to c36cf57 Compare March 6, 2019 12:09

MichelRottleuthner approved these changes Mar 6, 2019

View reviewed changes

MichelRottleuthner merged commit 3427dcd into RIOT-OS:master Mar 6, 2019

danpetry added this to the Release 2019.04 milestone Mar 11, 2019

MrKevinWeiss deleted the pr/addshellcommands branch March 17, 2025 10:27

Conversation

MrKevinWeiss commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

kaspar030 Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

kaspar030 Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

kaspar030 commented Feb 6, 2019

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

kaspar030 commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

MichelRottleuthner commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

kaspar030 commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

miri64 commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrKevinWeiss commented Feb 6, 2019

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Mar 6, 2019

MrKevinWeiss commented Feb 6, 2019 •

edited

Loading

miri64 Feb 6, 2019 •

edited

Loading

MichelRottleuthner commented Feb 6, 2019 •

edited

Loading

miri64 commented Feb 6, 2019 •

edited

Loading