script: logging: Fix small bugs and add support for live log parsing from a file #93107
Conversation
|
Hello @omrisarig13, and thank you very much for your first pull request to the Zephyr project! |
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
c47dcee to
b551461
Compare
|
Regarding the "Duplication on new code" - this is the case because of existing decision to split parser_v1 and parser_v3 to files that do not share base code - I tried to follow the current convention, which created this code-duplication. I think that this makes sense with the current architecture, but please let me know if you disagree. |
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
b551461 to
24db2bb
Compare
|
@dcpleung Thanks for the fast review. That's a very good idea - I've re-done the last commit, leaving the interface of I've added the relevant message to the migration guide for 4.2 - I'm not sure if that's the right place, or if it's too late and should be moved to 4.3. |
|
I think it is too late to get into 4.2. Could you move the migration guide entry to 4.3? |
|
correct, 4.3 is where this will go in |
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
24db2bb to
0fd9e6e
Compare
|
@dcpleung @nordic-krch - thank you for approving my PR. Is there anything more I need to do to get this in? It looks like some pipelines need to be manually triggered for them to run. |
|
@omrisarig13 I've enable checks. It won't get in until Zephyr release (tomorrow). |
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
0fd9e6e to
3766f9b
Compare
|
@nordic-krch @dcpleung The checker found some incompatibility issues in the python code with ruff - I've push a new version, where the only difference is the fixed compatibility of the python files. |
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
3766f9b to
8eb208e
Compare
|
@dcpleung - thank you for your help, and sorry for pinging you again. I've had the wrong format in the last patch, which caused the compliance to fail. It has now been fixed, and the checks have passed. |
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
8eb208e to
b5edc62
Compare
|
@dcpleung @nordic-krch - thanks for your approvals - there was a conflict in the migration guide file, happened because No other changes were done here. |
|
@nordic-krch Thanks for approving - can you enable the checks again, so we can merge this in? |
|
Hi @nordic-krch - it looks like the build failed with missing dependency of esptool, which I'm not familiar with (and not using myself/changing at all). Do you know what has happened here, and how I can solve this? Thanks! |
|
I think that has been fixed. Could you rebase so the CI can run again? |
Python does not support values of long-long (ll) or short-short (hh) in
string-formatting. The current parser is correctly removing the first "h"
or "l" from the format, to make it possible to correctly parse these
strings when they are in the dictionary.
However, the current implementation does not correctly handle the case of
having a number before the "ll" or "hh" argument - which is supported by
both C and Python.
This commit updates the parser to correctly handle these cases, by changing
the simple replace operator with a regex replace, which is able to
correctly handle this case.
Before this commit, the string of:
"This is my long variable: %08llX"
will stay the same, which will throw a ValueError when parsing this
dictionary message:
"ValueError: unsupported format character 'h' (0x68) at index ".
After this commit, the string will be correctly fixed to
"This is my long variable: %08lX"
which is parsed correctly.
Signed-off-by: Omri Sarig <[email protected]>
The current implementation of the serial log parser is running in a loop - every 2 seconds it checks if there is any information ready to be read, and in this case, it reads this information and parses it. In case the last packet of the information is not fully available, the script will read only the first part of the packet, and will fail when parsing, ending the run with an error. This should not the be the case, as it is not an error to have only part of the packet in the buffer. This commit fixes this problem - now, instead of failing because the parser does not have enough information, the parser will parse all the full packets it have, and keep the last, partial packet in the queue, to be parsed together with the next chunk of data. This is done by updating the log parsers to return the amount of parsed data when trying to parse the information, and updating the calling scripts to correctly handle this new return value. Additionally, the parser is now quietly failing in case of having a partial message, and throw an exception for any other kind of error in the parsing (instead of returning a boolean return code). In addition to the partial packet handling, the current commit also do the following, minor improvements: * parserlib now fails by throwing an exception, instead of exiting - this is done to make it possible to choose a different handling for the errors from the calling code, if needed. * The dictionary and parser are now created before the parse operation. This makes the uart parser more efficient, and also removes the previously duplicated messages of build id, target architecture and endianess (which was printed every time new information was received from the UART). Signed-off-by: Omri Sarig <[email protected]>
Create a new, generic parser, that is able to parse dictionary logs live - showing the messages as they are received from the device. This functionality previously existed only for reading the log from a UART. With the new script, the functionality is extended to be able to also read the log from a file or stdin. Additionally, the script is written in an extend-able way, making it simple to support more input sources in the future. The new script contains a better version of the live functionality than the old script of log_parser_uart script, therefore, the old script has been deprecated. The UART script is still available, and will work by invoking the new implementation with relevant arguments translation, however, it should ideally not be used any longer, and should be removed from a future release. Signed-off-by: Omri Sarig <[email protected]>
omrisarig13
force-pushed
the
logging/dictionary-from-stdin
branch
from
b5edc62 to
f0e9194
Compare
|
@dcpleung Thanks for the fast reply! I've rebased now and pushed the new version. |
|
|
Hi @omrisarig13! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
This PR contains 3 commits, all changing the log parser scripts.
The first commit add correct support to lengthed short-short and long-long arguments in the log ("%02hhX"). Currently, the log crashes when it tries to parse these valid formats, so this commit fixes this issue.
The second commit updates the current handling of non-full packets. This is an issue that exists when have live parsing of the logs - when the last read packet is not full, the parser would fail, and stop the parsing. This commit updates this handling to correctly handle this case, and keep parsing the packets correctly even when a non-full packet was received over the UART.
The third commit extends the functionality of the log_parser_uart to also support showing live log messages from files (and from stdin), making it possible to use the script with other interfaces than the UART interface.
This commit changes the file name, and the way it receives arguments - so it breaks backward compatibility of this script - I believe that this is an improvement to the script, and that it can be used by many more developers this way. I can re-work it so it is done in it's own script, if that makes more sense to you.
All the changes I've done are made to create support for these scripts to the project I'm working on, which have zephyr logging over RTT which are read with J-Link. I believe these are universal improvement to the script, which can be utilised by other people working with Zephyr as well.