Skip to content

Conversation

@OhadMeir
Copy link
Contributor

Tracked on [RSDEV-3262]

@OhadMeir OhadMeir requested a review from Nir-Az May 26, 2025 19:20
@Nir-Az Nir-Az requested review from Copilot and remibettan and removed request for Nir-Az May 27, 2025 06:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds support for handling firmware logs through a dictionary per module. It includes new unit tests for legacy and extended firmware logs, extends XML helper functions with module-specific capabilities, and updates the fw_logs_parser API and implementation to handle module IDs.

Reviewed Changes

Copilot reviewed 6 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
unit-tests/live/fw-logs/test-legacy.py Adds tests verifying legacy firmware log parsing with new XML support.
unit-tests/live/fw-logs/test-extended.py Adds tests verifying extended firmware log parsing with module-specific definitions.
src/fw-logs/fw-logs-xml-helper.h Declares a new helper function for module-specific file paths.
src/fw-logs/fw-logs-xml-helper.cpp Implements module-specific XML extraction for firmware logs.
src/fw-logs/fw-logs-parser.h Updates parser API to include module ID and adds a file-based options loader.
src/fw-logs/fw-logs-parser.cpp Implements module-specific log formatting options and updates error handling.
Files not reviewed (6)
  • unit-tests/live/fw-logs/extended-definitions.xml: Language not supported
  • unit-tests/live/fw-logs/extended-events1.xml: Language not supported
  • unit-tests/live/fw-logs/extended-events2.xml: Language not supported
  • unit-tests/live/fw-logs/extended-events3.xml: Language not supported
  • unit-tests/live/fw-logs/legacy-definitions.xml: Language not supported
  • unit-tests/live/fw-logs/legacy-events.xml: Language not supported
Comments suppressed due to low confidence (3)

src/fw-logs/fw-logs-parser.h:60

  • Consider renaming 'get_formating_options_from_file' to 'get_formatting_options_from_file' for improved clarity and consistency with standard terminology.
fw_logs_formatting_options get_formating_options_from_file( std::string path );

src/fw-logs/fw-logs-parser.cpp:167

  • The error message contains a typo ('formating' should be 'formatting'). Consider updating the message for clarity.
throw librealsense::invalid_value_exception( rsutils::string::from() << "FW logs parser expect one formating options, have " << num_of_sources );

src/fw-logs/fw-logs-parser.cpp:378

  • The concatenated string lacks a space between the source name and "doesn't". Consider adding a space for improved readability in the log message.
LOG_WARNING( rsutils::string::from() << "Source " << _source_id_to_name[source_id] << "doesn't have an xml file path, can't validate version" );

@OhadMeir OhadMeir force-pushed the fw_logs branch 2 times, most recently from 635cfd7 to aacce24 Compare May 28, 2025 12:58
Copy link
Contributor

@remibettan remibettan left a comment

Choose a reason for hiding this comment

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

LGTM

@OhadMeir OhadMeir merged commit c3e2f72 into realsenseai:development Jun 3, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants