RDKEMW-12562 : Move Bluetooth Mac update from Device Details to Bluetooth Service #419
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request disables automatic Bluetooth MAC address population during device bootup while retaining the capability for on-demand retrieval.
Changes:
- Commented out the
executeServiceRequest "bluetooth_mac"call in the "all" service request handler to prevent automatic Bluetooth MAC address retrieval at system bootup
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shibu-kv
left a comment
shibu-kv
left a comment
There was a problem hiding this comment.
Not able to get the context. Also ticket title and description is mentioning as test. No details of reason why we are disabling it.
Please update the ticket with relevant details.
nhanasi
changed the title
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.
|
Done @shibu-kv |
* RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scripts To C Implementation (#410) * Update Start_MaintenanceTasks.sh * Update lib/rdk/Start_MaintenanceTasks.sh Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update lib/rdk/Start_MaintenanceTasks.sh Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Start_MaintenanceTasks.sh * Update Start_MaintenanceTasks.sh * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Start_MaintenanceTasks.sh * Update Start_MaintenanceTasks.sh * Update Start_MaintenanceTasks.sh * Update Start_MaintenanceTasks.sh * Update Start_MaintenanceTasks.sh --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Saranya2421 <saranya.suvi@gmail.com> * Sysint 4.2.2 release changelog updates --------- Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Saranya2421 <saranya.suvi@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #### [4.2.2](https://github.com/rdkcentral/sysint/compare/4.2.1...4.2.2) | ||
|
|
||
| - RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scripts To C Implementation [`#410`](https://github.com/rdkcentral/sysint/pull/410) | ||
| - SERXIONE-8331 : Added viper_ipa to exceptions for /media/apps/sky/packages [`#420`](https://github.com/rdkcentral/sysint/pull/420) | ||
| - RDK-59247: cleaning up network scripts. [`#402`](https://github.com/rdkcentral/sysint/pull/402) | ||
| - RDKEMW-11363: [NM Dispatcher] Reduce repetitive logging [`#415`](https://github.com/rdkcentral/sysint/pull/415) | ||
| - Merge tag '4.2.1' into develop [`1df413f`](https://github.com/rdkcentral/sysint/commit/1df413f4f00e87a43eb092c6c57e302d754fba90) | ||
| - RDK-59247:Migrate Functionality In Network scripts To Core Modules [`8dd5ab9`](https://github.com/rdkcentral/sysint/commit/8dd5ab957251cfc80d6556560386019ca86088cb) |
There was a problem hiding this comment.
The CHANGELOG.md for version 4.2.2 does not include any reference to RDKEMW-12562 (the Bluetooth Mac update changes mentioned in the PR title). The changelog references other PRs and tasks but omits the primary purpose of this PR as stated in the title and description.
|
|
||
| if [ -n "$TriggerType" ] && [ "$TriggerType" -eq "$ON_DEMAND_LOG_UPLOAD" ]; then | ||
| logUploadLog "Application triggered on demand log upload" | ||
| sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "$TriggerType" 2>/dev/null | ||
| result=$? | ||
| if [ -x "$LOG_UPLOAD_BIN_PATH" ]; then | ||
| logUploadLog "Executing logupload binary: $LOG_UPLOAD_BIN_PATH" | ||
| "$LOG_UPLOAD_BIN_PATH" "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "ondemand" >> /opt/logs/dcmscript.log | ||
| result=$? | ||
| if [ "$result" -eq 0 ]; then | ||
| logUploadLog "Binary execution succeeded" | ||
| exit 0 | ||
| else | ||
| logUploadLog "Binary execution failed with result=$result; falling back to script" | ||
| sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "$TriggerType" 2>/dev/null | ||
| result=$? | ||
| fi | ||
| else | ||
| logUploadLog "logupload binary not found at $LOG_UPLOAD_BIN_PATH...executing script" | ||
| sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "$TriggerType" 2>&1 | ||
| result=$? | ||
| fi | ||
| else | ||
| logUploadLog "Log upload triggered from regular execution" | ||
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & | ||
| bg_pid=$! | ||
| wait $bg_pid | ||
| result=$? | ||
| if [ -x "$LOG_UPLOAD_BIN_PATH" ]; then | ||
| logUploadLog "Executing logupload binary: $LOG_UPLOAD_BIN_PATH" | ||
| nice -n 19 "$LOG_UPLOAD_BIN_PATH" "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" >> /opt/logs/dcmscript.log & | ||
| bg_pid=$! | ||
| wait $bg_pid | ||
| result=$? | ||
| if [ "$result" -eq 0 ]; then | ||
| logUploadLog "Binary execution succeeded" | ||
| return 0 | ||
| else | ||
| logUploadLog "Binary execution failed with result=$result; falling back to script" | ||
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & | ||
| bg_pid=$! | ||
| wait $bg_pid | ||
| result=$? | ||
| fi | ||
| else | ||
| logUploadLog "logupload binary not found at $LOG_UPLOAD_BIN_PATH...executing script" | ||
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & | ||
| bg_pid=$! | ||
| wait $bg_pid | ||
| result=$? | ||
| fi |
There was a problem hiding this comment.
The PR title and description indicate this change is about moving bluetooth_mac update from Device Details to Bluetooth Service. However, the major changes in this file are related to log upload binary migration (adding binary execution with fallback to script). These significant log upload changes are not mentioned in the PR description at all. This appears to be unrelated functionality mixed into the PR.
| "$LOG_UPLOAD_BIN_PATH" "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "ondemand" >> /opt/logs/dcmscript.log | ||
| result=$? | ||
| if [ "$result" -eq 0 ]; then | ||
| logUploadLog "Binary execution succeeded" | ||
| exit 0 | ||
| else | ||
| logUploadLog "Binary execution failed with result=$result; falling back to script" | ||
| sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "$TriggerType" 2>/dev/null |
There was a problem hiding this comment.
When using the binary on line 181, the TriggerType is hardcoded to "ondemand", but when falling back to the script on line 188, it uses the variable $TriggerType. This inconsistency could cause different behavior between binary and script execution. The binary should also receive $TriggerType as a parameter to maintain consistent behavior.
| nice -n 19 "$LOG_UPLOAD_BIN_PATH" "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" >> /opt/logs/dcmscript.log & | ||
| bg_pid=$! | ||
| wait $bg_pid | ||
| result=$? | ||
| if [ "$result" -eq 0 ]; then | ||
| logUploadLog "Binary execution succeeded" | ||
| return 0 | ||
| else | ||
| logUploadLog "Binary execution failed with result=$result; falling back to script" | ||
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & |
There was a problem hiding this comment.
In the regular execution path, the binary invocation on line 200 doesn't pass any TriggerType parameter at all, while the script fallback on line 209 also doesn't pass a TriggerType. However, this is inconsistent with the on-demand execution path (lines 181 and 188) where parameters are passed. The regular execution path binary should receive consistent parameters with the script to ensure proper functionality.
| return 0 | ||
| else | ||
| logUploadLog "Binary execution failed with result=$result; falling back to script" | ||
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & |
There was a problem hiding this comment.
There is trailing whitespace after the '&' character at the end of this line. This should be removed for code cleanliness.
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & | |
| nice -n 19 sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" & |
| sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "$TriggerType" 2>/dev/null | ||
| result=$? | ||
| fi | ||
| else | ||
| logUploadLog "logupload binary not found at $LOG_UPLOAD_BIN_PATH...executing script" | ||
| sh $LOGUPLOAD_SCRIPT "$tftp_server" 1 1 "$uploadOnReboot" "$upload_protocol" "$upload_httplink" "$TriggerType" 2>&1 |
There was a problem hiding this comment.
Inconsistent error handling: line 188 redirects stderr to /dev/null with '2>/dev/null', while line 193 redirects both stdout and stderr with '2>&1'. This difference in error handling between the two code paths could lead to missing error information in logs. Consider using consistent redirection, preferably '2>&1' to capture all output in logs.
Update getDeviceDetails.sh to move the bluetooth_mac population at bootup and move it as part of bluetooth service