Support FW logs extended format #12688
Conversation
…xtended format in parser
…at xml. fw-log-xml-helper updated, fw-logs-formatting-options gets needed data from it
…andle extended format in parser
src/fw-logs/fw-log-data.cpp
Outdated
| return (uint32_t)_timestamp; | ||
| } | ||
| const fw_log_binary * log_binary = reinterpret_cast< const fw_log_binary * >( logs_buffer.data() ); | ||
| if( log_binary->magic_number == 0xA5) |
There was a problem hiding this comment.
This is how we distinguish d400 from d500?
magic number was changed?
There was a problem hiding this comment.
We need to support get_severity from the log entry binary data, in case no parser is available.
This is the easiest way I found doing so.
…ce format xml. fw-log-xml-helper updated, fw-logs-formatting-options gets needed data from it
src/fw-logs/fw-logs-parser.cpp
Outdated
| { | ||
| _fw_logs_formating_options.initialize_from_xml(); | ||
| fw_logs_xml_helper helper; |
There was a problem hiding this comment.
Can you add some comments about what is being done here?
TOC file and such
There was a problem hiding this comment.
Sorry but can you add the actual example in a comment?
This way if someone will ask how it should look like we can see it in the code?
…not need new definitions file
src/firmware_logger_device.h
Outdated
| @@ -1,66 +1,85 @@ | |||
| // License: Apache 2.0. See LICENSE file in root directory. | |||
| // Copyright(c) 2020 Intel Corporation. All Rights Reserved. | |||
| // Copyright(c) 2024 Intel Corporation. All Rights Reserved. | |||
There was a problem hiding this comment.
I don't think you need to change the date when you modify a file
There was a problem hiding this comment.
It is something me and others have been doing as of lately. Trying to google if this is a good practice, I came across mixed opinions if this is necessary or even good to do this.
@Nir-Az, this is a legal issue, who should we contact about this?
There was a problem hiding this comment.
Normally I followed a role that a new file gets new year and modified not.
But I am not sure if any restrictions or best practice here.
I will check and update
There was a problem hiding this comment.
As instructed, edited files should show range of years. Fixing here to 2020-2024
src/fw-logs/fw-log-data.cpp
Outdated
| @@ -1,70 +1,91 @@ | |||
| // License: Apache 2.0. See LICENSE file in root directory. | |||
| // Copyright(c) 2019 Intel Corporation. All Rights Reserved. | |||
| // Copyright(c) 2024 Intel Corporation. All Rights Reserved. | |||
src/fw-logs/fw-log-data.h
Outdated
| uint32_t timestamp; | ||
| }; | ||
|
|
||
| struct fw_log_binary | ||
| struct extended_fw_log_binary : public fw_log_binary |
There was a problem hiding this comment.
Could you change the "extended" to "d500"?
Would it be right?
There was a problem hiding this comment.
I tried to avoid model names as it might not be correct.
The new (extended) format aims to be future proof (within reason). What if this format would be used for D600 models? What about old L500 models using the old format.
But I agree that "legacy" and "extended" are also not that good, as further changes to the format might make these names obsolete.
Lets talk tomorrow and decide on the preferred naming approach.
There was a problem hiding this comment.
After discussion the naming convention is - "legacy" parser/device/etc.. will have no prefix. New format classes will use "extended_" prefix.
This cause some weird naming in the implementation as I had firmware_logger_device inheriting extended_firmware_logger_device. I have changed the implementation moving functions from base classes to inheritors to reverse this. Now "extended_" classes inherit the no prefix classes.
|
|
||
| return false; | ||
| } | ||
| document->parse< 0 >( buffer.data() ); // Might throw |
| { | ||
| if (_xml_content.empty()) | ||
| return false; | ||
| xml_node<> * get_first_node( const xml_document<> * document ) |
There was a problem hiding this comment.
what do you mean by xml_node<> ? is it partial specialization? It does not seem so, since the xml_node class if not template.
There was a problem hiding this comment.
xml_node is a template with default type.
template
class xml_node: public xml_base
Tracked on [RSDEV-1642] + [RSDEV-1643] + [RSDEV-1645]
This PR introduces the extended FW log structure.
It introduces the option that we will get FW logs from several sources, each source sending logs from several modules with dynamic number and type of parameters.
API was updated to get FW log module. To minimize API change it was decided that to get the log source we will use the
rs2_get_fw_log_parsed_thread_nameexisting API.Parser now uses a definition XML that points to each source events log parser XML (HWLoggerEventsDS5.xml for D400)