Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional logging to help diagnose failures in FrontPanelIndicator::setColor(...) (RDKEMW-17349).
Changes:
- Logs the requested color ID/name when
setColor(Color, toPersist)is called. - Logs when
setColoris invoked in unsupported color modes. - Logs supported/invalid color IDs during validation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| void FrontPanelIndicator::setColor(const FrontPanelIndicator::Color & color,bool toPersist) | ||
| { | ||
| INT_INFO("[%s:%d] Color id = %d, color name = %s", __FUNCTION__, __LINE__, color.getId(), color.getName().c_str()); |
There was a problem hiding this comment.
INT_INFO already logs file/line/function via ds_log (see ds/dslogger.cpp:66-72). Including [%s:%d] with __FUNCTION__/__LINE__ in the message is redundant and makes logs noisier; consider removing that prefix and logging just the relevant fields (or moving this to INT_DEBUG if it's primarily for troubleshooting).
| INT_INFO("[%s:%d] Color mode is %d, API not supported", __FUNCTION__, __LINE__, _colorMode); | ||
| throw UnsupportedOperationException("This API not supported for the color mode"); |
There was a problem hiding this comment.
The log/exception text is grammatically incorrect and not very actionable ("This API not supported..."). Consider changing it to "This API is not supported for color mode X" (including _colorMode), so failures are clearer in telemetry and in exception reports.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -525,10 +530,12 @@ void FrontPanelIndicator::setColor(uint32_t color,bool toPersist) | |||
| bool isValidColor = false; | |||
| if(_colorMode == 0 || _colorMode == 2) | |||
| { | |||
There was a problem hiding this comment.
Log message says "setLED" but this is FrontPanelIndicator::setColor(uint32_t). This looks like a copy/paste error and will confuse diagnostics. Also, since INT_INFO already prints file/line/function, drop the embedded "[%s:%d]" prefix and include the actual _colorMode value in the message if needed for troubleshooting.
| { | |
| INT_INFO("setColor(uint32_t): API not supported for color mode %d", _colorMode); |
No description provided.