XER10-2389: Gateway LED lite solid white in WFO mode - #4
Conversation
There was a problem hiding this comment.
Pull request overview
Adds RBUS-driven WAN primary/backup interface event handling so the LED manager can react to WFO failover/fallback scenarios (e.g., avoiding incorrect solid-white behavior by emitting explicit “backup active” / “primary active” events).
Changes:
- Subscribes to
Device.X_RDK_WanManager.InterfaceActiveStatusand translates status updates into new LED events (eWanBackupActive,eWanPrimaryActive). - Adjusts build wiring so the TR-181 middle-layer (including RBUS handler) is built/linked consistently, and gates WebConfig-specific RBUS elements behind
LEDMGR_WEBCONFIG. - Exposes
handle_event()for use by the RBUS handler and adds locking around event handling.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| source/TR-181/middle_layer_src/Makefile.am | Updates include/link flags; makes json-schema wrapper link conditional and (should) align feature toggles with LEDMGR_WEBCONFIG. |
| source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c | Adds WAN interface status RBUS subscription and emits new LED events; wraps WebConfig-related RBUS elements with LEDMGR_WEBCONFIG. |
| source/TR-181/middle_layer_src/ledmgr_plugin_main_apis.c | Guards WebConfig init call behind LEDMGR_WEBCONFIG. |
| source/Makefile.am | Builds TR-181 subdir unconditionally to support RBUS handler usage. |
| source/LedManager/Makefile.am | Makes LedManager depend on the TR-181 middle-layer library unconditionally. |
| source/LedManager/led_manager.h | Adds new event enum values for WAN backup/primary activity. |
| source/LedManager/led_manager.c | Always includes RBUS handler header and always initializes RBUS. |
| source/LedManager/led_manager_events.c | Exposes handle_event() and adds mutex protection around event processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:840
- LedMgr_Rbus_Init subscribes to Device.X_RDK_WanManager.InterfaceActiveStatus, but LedMgr_Rbus_Exit never unsubscribes from it before closing the rbus handle. This can leak subscriptions / generate errors on shutdown. Also, LedMgr_Rbus_UnSubscribeDML should only run when LEDMGR_WEBCONFIG subscriptions were actually created.
ANSC_STATUS LedMgr_Rbus_Exit()
{
CcspTraceInfo(("%s %d - LedMgr_RbusExit called\n", __FUNCTION__, __LINE__ ));
#ifdef LEDMGR_WEBCONFIG
rbus_unregDataElements(rbusHandle, NUM_OF_RBUS_PARAMS, ledMgrRbusDataElements);
#endif
LedMgr_Rbus_UnSubscribeDML();
rbus_close(rbusHandle);
return ANSC_STATUS_SUCCESS;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
source/TR-181/middle_layer_src/Makefile.am:32
- The added include path points to a non-existent directory (middle_layer_src/json_schema_validator_wrapper). The repo’s header lives under source/TR-181/json_schema_validator_wrapper, so WebConfig builds may still fail to find json_schema_validator_wrapper.h.
libCcspLedManager_middle_layer_src_la_CPPFLAGS += -I$(top_srcdir)/source/TR-181/middle_layer_src/json_schema_validator_wrapper
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/Makefile.am:32
- The json_schema_validator_wrapper header lives under source/TR-181/json_schema_validator_wrapper, but this Makefile adds an include path to source/TR-181/middle_layer_src/json_schema_validator_wrapper (which doesn't exist). When LEDMGR_WEBCONFIG is enabled, including json_schema_validator_wrapper.h will fail to compile unless another global include path happens to provide it.
libCcspLedManager_middle_layer_src_la_CPPFLAGS += -I$(top_srcdir)/source/TR-181/middle_layer_src/json_schema_validator_wrapper
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:779
- This is an unsubscribe error path, but the log says "Failed to Subscribe", which is misleading during troubleshooting.
CcspTraceError(("%s %d - Failed to Subscribe %s, Error=%s \n", __FUNCTION__, __LINE__, LEDMGR_WEBCONFIG_FULLJSON_DATA, rbusError_ToString(ret)));
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:841
- The function was renamed to LedMgr_Rbus_Exit, but this log string still says "LedMgr_RbusExit", making logs harder to correlate with the actual symbol name.
CcspTraceInfo(("%s %d - LedMgr_RbusExit called\n", __FUNCTION__, __LINE__ ));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:723
- On the error path where
ifTypeis empty, the function returns without freeingactive_status, leaking the duplicated buffer. Freeactive_statusbefore returning.
if(strlen(ifType) == 0)
{
CcspTraceError(("%s:%d Cannot determine interface type\n",__FUNCTION__, __LINE__));
return IF_NONE_ACTIVE;
}
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:790
WanInterfaceStatusHandlercallsGetActiveInterfaceType(status)twice, which duplicates parsing work (and duplicates allocations via strdup). Compute it once and branch on the result.
/* HOTSPOT,0|DOCSIS,0|WANOE,1|DSL,0|REMOTE_LTE,0 */
if(GetActiveInterfaceType(status) == IF_PRIMARY_ACTIVE)
{
if(wan_backup_status == 0)
{
CcspTraceInfo(("%s:%d Ignoring Notification for %s\n",__FUNCTION__, __LINE__, eventName));
source/LedManager/led_manager_events.c:108
handle_eventis now a non-static symbol used from another translation unit (viaextern), but there is no corresponding prototype in a shared header (e.g., led_manager_events.h). Exporting functions without a header risks signature drift and compiler warnings in future call sites.
int handle_event(cpe_event_t event)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.h:60
- The header now declares static helpers using RBUS types (rbusHandle_t/rbusEvent_t), but this header does not include <rbus.h>. Files like source/LedManager/led_manager.c include this header without including rbus, so this will fail to compile. Since these helpers are file-local (static) in ledmgr_rbus_handler_apis.c, they should not be declared in the public header.
static InterfaceStatusType GetActiveInterfaceType(const char *interface_active_status);
static void WanInterfaceStatusHandler(rbusHandle_t handle, rbusEvent_t const* event, rbusEventSubscription_t* subscription);
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:872
- If allocating g_LedMgr.data fails, LedMgr_Rbus_Init returns without closing the RBUS handle opened earlier, leaking the handle/file descriptors.
g_LedMgr.data = (char*)malloc(LED_DATA_MEM_SIZE);
if ( g_LedMgr.data == NULL )
{
AnscTraceError(("%s:%d:: Unable to allocate dynamic memory!\n", __FUNCTION__, __LINE__ ));
return ANSC_STATUS_FAILURE;
…as backup mode Reason for change: Send primary and backup events from rbus handler and handle the events Test Procedure: Test WFO use cases Risks: Low Signed-off-by: biju.vijayanindiradevi@sky.uk XER10-2389: Gateway LED lite solid white in WFO mode while using XLE as backup mode Reason for change: Send primary and backup events from rbus handler and handle the events Test Procedure: Test WFO use cases Risks: Low Signed-off-by: biju.vijayanindiradevi@sky.uk
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/Makefile.am:36
- The json_schema_validator_wrapper header lives under source/TR-181/json_schema_validator_wrapper, but the include path added here points to source/TR-181/middle_layer_src/json_schema_validator_wrapper (which doesn’t exist). This will break webconfig builds when ledmgr_rbus_handler_apis.c includes json_schema_validator_wrapper.h.
libCcspLedManager_middle_layer_src_la_CPPFLAGS += -I$(top_srcdir)/source/TR-181/middle_layer_src/json_schema_validator_wrapper
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.h:60
- Declaring these helpers as static in the public header will trigger "declared static but never defined" warnings in any translation unit that includes this header (e.g., led_manager.c), and the build uses -Werror. These prototypes should be removed from the header (or made non-static and exposed intentionally).
static InterfaceStatusType GetActiveInterfaceType(const char *interface_active_status);
static void WanInterfaceStatusHandler(rbusHandle_t handle, rbusEvent_t const* event, rbusEventSubscription_t* subscription);
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:907
- The function was renamed to LedMgr_Rbus_Exit, but this log line still says "LedMgr_RbusExit called", which is confusing during debugging.
CcspTraceInfo(("%s %d - LedMgr_RbusExit called\n", __FUNCTION__, __LINE__ ));
…as backup mode Reason for change: Send primary and backup events from rbus handler and handle the events Test Procedure: Test WFO use cases Risks: Low Signed-off-by: biju.vijayanindiradevi@sky.uk
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
source/TR-181/middle_layer_src/Makefile.am:37
- The json_schema_validator_wrapper include path points to a directory that doesn’t exist in this repo (the header is under source/TR-181/json_schema_validator_wrapper). With FEATURE_LEDMGR_WEBCONFIG enabled, this will break compilation of includes like "json_schema_validator_wrapper.h".
if FEATURE_LEDMGR_WEBCONFIG
libCcspLedManager_middle_layer_src_la_CPPFLAGS += -I$(top_srcdir)/source/TR-181/middle_layer_src/json_schema_validator_wrapper
libCcspLedManager_middle_layer_src_la_LDFLAGS += -ljson_schema_validator_wrapper
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:909
- The trace message still says "LedMgr_RbusExit" even though the function is now named LedMgr_Rbus_Exit(), which can be confusing when grepping logs.
CcspTraceInfo(("%s %d - LedMgr_RbusExit called\n", __FUNCTION__, __LINE__ ));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
source/TR-181/middle_layer_src/Makefile.am:37
- The json_schema_validator_wrapper include path points to a non-existent directory (the header is under source/TR-181/json_schema_validator_wrapper). This will break FEATURE_LEDMGR_WEBCONFIG builds when compiling ledmgr_rbus_handler_apis.c with LEDMGR_WEBCONFIG enabled.
if FEATURE_LEDMGR_WEBCONFIG
libCcspLedManager_middle_layer_src_la_CPPFLAGS += -I$(top_srcdir)/source/TR-181/middle_layer_src/json_schema_validator_wrapper
libCcspLedManager_middle_layer_src_la_LDFLAGS += -ljson_schema_validator_wrapper
source/TR-181/middle_layer_src/ledmgr_rbus_handler_apis.c:911
- The trace message says "LedMgr_RbusExit called" but the function was renamed to LedMgr_Rbus_Exit; this makes logs harder to correlate with the actual symbol/function name.
ANSC_STATUS LedMgr_Rbus_Exit()
{
CcspTraceInfo(("%s %d - LedMgr_RbusExit called\n", __FUNCTION__, __LINE__ ));
#ifdef LEDMGR_WEBCONFIG
rbus_unregDataElements(rbusHandle, NUM_OF_RBUS_PARAMS, ledMgrRbusDataElements);
source/LedManager/Makefile.am:39
- led_manager_events.c now uses pthread_mutex_lock/unlock unconditionally (not under FEATURE_LEDMGR_WEBCONFIG), but this Makefile only links -lpthread inside the FEATURE_LEDMGR_WEBCONFIG block. On toolchains that don't pull pthread transitively, non-webconfig builds may fail to link with undefined pthread_* symbols. Link pthread unconditionally for rdkledmanager now that it directly uses pthread APIs.
${top_builddir}/source/TR-181/middle_layer_src/libCcspLedManager_middle_layer_src.la
rdkledmanager_CFLAGS = -Werror -D_ANSC_LINUX -D_ANSC_USER -D_ANSC_LITTLE_ENDIAN_ -DFEATURE_SUPPORT_RDKLOG $(DBUS_CFLAGS) $(SYSTEMD_CFLAGS)
rdkledmanager_SOURCES = led_manager.c led_manager_events.c led_manager_utils.c led_manager_dbus_utils.c
rdkledmanager_LDFLAGS = -lccsp_common -lrdkloggers -ljson-c $(DBUS_LIBS) $(SYSTEMD_LDFLAGS) -lsysevent -lhal_platform -lledmanager -lsky_hub -lsyscfg
XER10-2389: Gateway LED lite solid white in WFO mode
Reason for change: Send primary and backup events from rbus handler and handle the events
Test Procedure: Test WFO use cases
Risks: Low
Signed-off-by: biju.vijayanindiradevi@sky.uk