Updated the Table of diagnostic messages

robotology / icub-firmware-shared

Protocols and Other Stuff Used both by iCub Firmware and iCub Software

GNU Lesser General Public License v2.1

4 stars 18 forks source link

Updated the Table of diagnostic messages #84

Closed valegagge closed 1 year ago

valegagge commented 1 year ago

Related to the https://github.com/robotology/icub-main/pull/886 PR, we updated the table containing the diagnostic message: we removed the part of the message that explained how to interpret the parameters because now it is performed automatically by the new formatter.

cc @MSECode

valegagge commented 1 year ago

I put in draft until the PR https://github.com/robotology/icub-firmware-shared/pull/83 is not merged. So I can rabase and update the icub-fw shared. then I'll open again it.

marcoaccame commented 1 year ago

Hi @valegagge: do we need to change the content of the LUT w/ error messages in here? Because the firmware uses them to print over the debug port what is happening. If you change them we miss some info in debug.

Maybe you could implement a new LUT for use by icub-main w/ cleaned text.

valegagge commented 1 year ago

Hi @marcoaccame ,

Which is the debug port?

I think it doesn't make sense to have two different LUT.

Let's discuss f2f about this. :)

valegagge commented 1 year ago

Hi @marcoaccame agian:),

I just want to highlight, that I removed only the part of the string that explains how to interpret the parameters. If you need this information, I suggest to put in a documentation page and not left in the code.

What do you think?

marcoaccame commented 1 year ago

I just want to highlight, that I removed only the part of the string that explains how to interpret the parameters. If you need this information, I suggest to put in a documentation page and not left in the code.

Yes, that is fine. It is surely necessary some documentation in order to define several operations. Such as how to interpret the params if the parser of icub-main is not available as in the firmware which uses the LUT as a parser, but also how to add new messages. I mean: if I add a new message or I change the value in a {par16, par64}, where to change code? It is a matter which crosses this PR, the one in icub-main and new material to prepare in documentation.

valegagge commented 1 year ago

If we add or change the parameters I hope we discuss them together since we are going to improve the diagnostic also in the fw.

Anyway, the documentation should be done as a lot of other documentation missing for fw.

We can take care of writing documentation for diagnostics, but now it is not possible.

marcoaccame commented 1 year ago

Hi @valegagge, I think that the change proposed in here as a comment to the file that contains LUT w/ the error string can solve several needs:

to have a shorted string in icub-main,
to retain a longer string for use of some testing functionalities inside icub-firmware,
to keep the interpretation of the parameters p16 and p64 for reference to the icub-main parser and also for the benefit of the documentation.

valegagge commented 1 year ago

I think we need to have a f2f discussion.

After a f2f discussion with @marcoaccame, we decided to add the extra string in order to facilitate the fw debug. The leave the parser rules string near the entry in the LUT is not sufficient.

In the future, we can check together if it is possible to create a slim parser that can print human-readable messages both on fw and on yarprobotinterface

marcoaccame commented 1 year ago

you also need also to rebase this fork vs robotology/devel and solve the conflict on the versioning.

valegagge commented 1 year ago

Today @MSECode and I tested this PR and its related PR on icub-main https://github.com/robotology/icub-main/pull/886.

All work fine. So we can merge.

We increase the version to 1.35.1

cc @marcoaccame