Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are small but concrete issues in the newly added lines (a typo and an identifier/correlation improvement) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a deterministic log marker to help operational monitoring detect hotspot clients that are disassociated after a DHCP flow fails (no ACK within the timer window), as requested in RDKCOM-5639.
Changes:
- Emit a new
CcspTraceInfomarker when DHCP ACK is not observed and a reject/disassociate RBus event is published. - Include the affected client identifier in the marker log line.
File summaries
| File | Description |
|---|---|
| source/hotspotfd/dhcpsnooper.c | Adds an INFO log marker during DHCP-failure-triggered client disassociation flow. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…nts due to DHCP failure
Reason for change: Add a marker string to monitor Disassociated Hotspot clients due to DHCP failure
Test Procedure:
1.Change the endpoint ip address and make sure Wag doen't respond to DHCP discover.
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.PrimaryRemoteEndpoint string 75.75.75.75
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.SecondaryRemoteEndpoint string 75.75.76.76
2. Verify Marker string is logged in /rdklogs/logs/Hotspotlog.txt.0 file
if hotspot client doesn't get the ip from WAG
Risks : None
Priority : P2
There was a problem hiding this comment.
🟡 Changes recommended
The new marker is currently gated on RBUS publish success, which can prevent the marker from being logged in DHCP-failure scenarios described by the PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
…nts due to DHCP failure
Reason for change: Add a marker string to monitor Disassociated Hotspot clients due to DHCP failure
Test Procedure:
1.Change the endpoint ip address and make sure Wag doen't respond to DHCP discover.
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.PrimaryRemoteEndpoint string 75.75.75.75
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.SecondaryRemoteEndpoint string 75.75.76.76
2. Verify Marker string is logged in /rdklogs/logs/Hotspotlog.txt.0 file
if hotspot client doesn't get the ip from WAG
Risks : None
Priority : P2
…nts due to DHCP failure
Reason for change: Add a marker string to monitor Disassociated Hotspot clients due to DHCP failure
Test Procedure:
1.Change the endpoint ip address and make sure Wag doen't respond to DHCP discover.
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.PrimaryRemoteEndpoint string 75.75.75.75
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.SecondaryRemoteEndpoint string 75.75.76.76
2. Verify Marker string is logged in /rdklogs/logs/Hotspotlog.txt.0 file
if hotspot client doesn't get the ip from WAG
Risks : None
Priority : P2
| //Added marker to track the disassociated client upon DHCP failure | ||
| CcspTraceInfo(("DHCP_FAILED_AND_CLIENT_DISASSOCIATED: %s\n",mac_str)); | ||
| snprintf(telemetry_buf, sizeof(telemetry_buf), "%s", mac_str); | ||
| t2_event_s("DHCP_FAILED_AND_CLIENT_DISASSOCIATED", telemetry_buf); |
…nts due to DHCP failure
Reason for change: Add a marker string to monitor Disassociated Hotspot clients due to DHCP failure
Test Procedure:
1.Change the endpoint ip address and make sure Wag doen't respond to DHCP discover.
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.PrimaryRemoteEndpoint string 75.75.75.75
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.SecondaryRemoteEndpoint string 75.75.76.76
2. Verify Marker string is logged in /rdklogs/logs/Hotspotlog.txt.0 file
if hotspot client doesn't get the ip from WAG
Risks : None
Priority : P2
There was a problem hiding this comment.
🟡 Changes recommended
The test bypasses the production timeout path and does not verify marker ordering or MAC propagation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
…pot clients due to DHCP failure" This reverts commit 00551f8.
There was a problem hiding this comment.
🟢 Approval recommended
The only remaining finding is a minor documentation nit.
Review details
Suppressed comments (1)
source/hotspotfd/dhcpsnooper.c:1245
- This adds a new T2 marker, but the repository's telemetry inventories at
README.md:585anddocs/README.md:585enumerate the marker names and omitDHCP_FAILED_AND_CLIENT_DISASSOCIATED. Please add the new marker to both tables so operators and telemetry consumers can discover this event.
t2_event_s("DHCP_FAILED_AND_CLIENT_DISASSOCIATED", telemetry_buf);
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
pradeeptakdas
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RDKCOM-5639:Add a marker string to monitor Disassociated Hotspot clients due to DHCP failure
Reason for change: Add a marker string to monitor Disassociated Hotspot clients due to DHCP failure
Test Procedure:
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.PrimaryRemoteEndpoint string 75.75.75.75
dmcli eRT setv Device.X_COMCAST-COM_GRE.Tunnel.1.SecondaryRemoteEndpoint string 75.75.76.76
if hotspot client doesn't get the ip from WAG
Risks : None
Priority : P2