Skip to content

Rebase#856

Open
Abhinavpv28 wants to merge 30 commits into
feature/rdm_crash_lengthfrom
develop
Open

Rebase#856
Abhinavpv28 wants to merge 30 commits into
feature/rdm_crash_lengthfrom
develop

Conversation

@Abhinavpv28
Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings May 15, 2026 07:16
@Abhinavpv28 Abhinavpv28 requested review from a team as code owners May 15, 2026 07:16
Copy link
Copy Markdown
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 PR updates Chrony runtime configuration behavior and defaults, adjusts Chrony telemetry timestamp formatting, and bumps the remote debugger recipe revision.

Changes:

  • Adds default Chrony makestep and bootstrap NTP servers.
  • Reworks Chrony config generation around per-host settings and makestep handling.
  • Updates remote debugger PV/SRCREV to 1.3.4.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
recipes-support/chrony/files/rdk_chrony.conf Adds default makestep and bootstrap NTP server entries.
recipes-support/chrony/files/chrony-conf-update.sh Reworks dynamic Chrony config generation, makestep handling, and fallback behavior.
recipes-support/chrony/files/chrony_tracking.sh Changes telemetry timestamp format to UTC ISO-style format.
recipes-extended/remotedebugger/remotedebugger.bb Bumps remote debugger version and source revision.
Comments suppressed due to low confidence (3)

recipes-support/chrony/files/chrony-conf-update.sh:200

  • Missing settings now disable iburst, even though the previous generated config and the new default rdk_chrony.conf both use iburst for every server. Any partner or bootstrap host without a populated ntpHost*Settings value will start without iburst, which can significantly delay initial time synchronization on boot; the missing-field default should preserve the existing iburst behavior unless the setting explicitly disables it.
    # Normalise iburst: only literal "true" enables it
    [ "$s_iburst" = "true" ] || s_iburst="false"

recipes-support/chrony/files/chrony-conf-update.sh:147

  • The retry loop was removed, so an empty result from getPartnerProperty causes an immediate bootstrap/default fallback. chronyd.service only orders after tr69hostif/network-up, and the previous code allowed up to five attempts, so transient RFC/TR-181 availability during boot can now cause partner NTP settings to be skipped for the whole chronyd start.
ntpLog "Retrieve NTP Server URL from /lib/rdk/getPartnerProperty.sh..."
get_ntp_hosts

if ! ( [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ] ); then
    ntpLog "TR-181 returned empty NTP server list; falling back to /opt/secure/RFC/bootstrap.ini..."

recipes-support/chrony/files/chrony-conf-update.sh:162

  • When no current partner/bootstrap URLs are available, this path preserves whatever server/pool lines are already in /etc/rdk_chrony.conf. Because that file is dynamically rewritten and packaged as a conffile, it may contain stale partner entries (or an older file with no default servers) rather than the new build-time defaults, leaving chronyd pointed at obsolete sources or with no source at all.
if ! ( [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ] ); then
    if [ -n "$maxstep" ]; then
        ntpLog "No partner NTP URLs found; updating makestep from TR-181 and retaining build-time server config"
        strip_makestep_only "$CHRONY_CONF"
        write_makestep

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

PR = "r0"
PACKAGE_ARCH = "${MIDDLEWARE_ARCH}"
SRCREV = "7151d208ced521de7ee7dee66cca6aea951e95b3"
SRCREV = "4941e3899ab9217382788296594cd14350586268"
Comment on lines +54 to +58
settings1=`/lib/rdk/getPartnerProperty.sh ntpHost1Settings`
settings2=`/lib/rdk/getPartnerProperty.sh ntpHost2Settings`
settings3=`/lib/rdk/getPartnerProperty.sh ntpHost3Settings`
settings4=`/lib/rdk/getPartnerProperty.sh ntpHost4Settings`
settings5=`/lib/rdk/getPartnerProperty.sh ntpHost5Settings`
Copilot AI review requested due to automatic review settings May 17, 2026 14:04
Copy link
Copy Markdown
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

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

recipes-support/chrony/files/chrony-conf-update.sh:119

  • This uses the same predictable /tmp pattern in a root-run path. If the temporary file is pre-created or redirected, the script can overwrite unintended content or corrupt /etc/rdk_chrony.conf; create the temp file securely and only replace the config after the filter command succeeds.
strip_makestep_only() {
    local conf_file="$1"
    local tmp_conf="/tmp/rdk_chrony.conf.$$"

    awk '!/^[[:space:]]*makestep[[:space:]]/' \
        "$conf_file" > "$tmp_conf"
    cat "$tmp_conf" > "$conf_file"

Comment on lines +158 to +164
if ! ( [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ] ); then
if [ -n "$maxstep" ]; then
ntpLog "No partner NTP URLs found; updating makestep from TR-181 and retaining build-time server config"
strip_makestep_only "$CHRONY_CONF"
write_makestep
else
echo "makestep 1.0 3" >> "$CHRONY_CONF"
ntpLog "NTPMaxstep value '$maxstep' is invalid, using default makestep 1.0 3 in $CHRONY_CONF"
ntpLog "No partner NTP URLs found; retaining build-time default configuration in $CHRONY_CONF"
Comment on lines +143 to +148
ntpLog "Retrieve NTP Server URL from /lib/rdk/getPartnerProperty.sh..."
get_ntp_hosts

if [ "$minPoll" -gt "$maxPoll" ]; then
ntpLog "ERROR: minPoll ($minPoll) is greater than maxPoll ($maxPoll), resetting both to defaults ($DEFAULT_MINPOLL/$DEFAULT_MAXPOLL)"
minPoll="$DEFAULT_MINPOLL"
maxPoll="$DEFAULT_MAXPOLL"
if ! ( [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ] ); then
ntpLog "TR-181 returned empty NTP server list; falling back to /opt/secure/RFC/bootstrap.ini..."
get_ntp_hosts_from_bootstrap
Comment on lines +199 to +200
# Normalise iburst: only literal "true" enables it
[ "$s_iburst" = "true" ] || s_iburst="false"
Comment on lines +100 to +108
local conf_file="$1"
local tmp_conf="/tmp/rdk_chrony.conf.$$"

ntpLog "Retrieve NTP Server URL from /lib/rdk/getPartnerProperty.sh..."
while [ "$attempts" -le "$max_attempts" ]; do
# Remove all server, pool, and makestep lines so every run starts from a
# clean slate — partner entries from a previous run are cleared along with
# build-time defaults, making a separate deduplication step unnecessary.
awk '!/^[[:space:]]*(server|pool|makestep)[[:space:]]/' \
"$conf_file" > "$tmp_conf"
cat "$tmp_conf" > "$conf_file"
@@ -42,33 +40,6 @@ ntpLog()
echo "`/bin/timestamp` : $0: $*" >> "$LOG_FILE"
}

Copilot AI review requested due to automatic review settings May 18, 2026 12:27
Copy link
Copy Markdown
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

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

recipes-support/chrony/files/chrony-conf-update.sh:200

  • When ntpHostXSettings is absent (including the bootstrap.ini fallback path), this defaults s_iburst to false, so generated server/pool lines omit iburst. The build-time defaults and the previous generator always included iburst, so devices that only provide hostnames will regress to slower initial time synchronization unless the missing setting preserves the prior default.
    # Normalise iburst: only literal "true" enables it
    [ "$s_iburst" = "true" ] || s_iburst="false"

recipes-support/chrony/files/chrony-conf-update.sh:166

  • This no-host path leaves whatever server/pool entries are already in /etc/rdk_chrony.conf, rather than restoring the packaged defaults described by the log/comment. After any prior partner update, the packaged default servers have been stripped and replaced, so a later empty TR-181/bootstrap response will keep stale partner servers indefinitely instead of falling back to the new default bootstrap servers.
if ! ( [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ] ); then
    if [ -n "$maxstep" ]; then
        ntpLog "No partner NTP URLs found; updating makestep from TR-181 and retaining build-time server config"
        strip_makestep_only "$CHRONY_CONF"
        write_makestep
    else
        ntpLog "No partner NTP URLs found; retaining build-time default configuration in $CHRONY_CONF"
    fi
    exit 0

Comment on lines +143 to +148
ntpLog "Retrieve NTP Server URL from /lib/rdk/getPartnerProperty.sh..."
get_ntp_hosts

if [ "$minPoll" -gt "$maxPoll" ]; then
ntpLog "ERROR: minPoll ($minPoll) is greater than maxPoll ($maxPoll), resetting both to defaults ($DEFAULT_MINPOLL/$DEFAULT_MAXPOLL)"
minPoll="$DEFAULT_MINPOLL"
maxPoll="$DEFAULT_MAXPOLL"
if ! ( [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ] ); then
ntpLog "TR-181 returned empty NTP server list; falling back to /opt/secure/RFC/bootstrap.ini..."
get_ntp_hosts_from_bootstrap
Copilot AI review requested due to automatic review settings May 21, 2026 04:44
Copy link
Copy Markdown
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

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.


[Service]
Type=oneshot
RemainAfterExit=yes
[Service]
Type=oneshot
Type=simple
RemainAfterExit=yes
Comment on lines +36 to +40
PACKAGECONFIG[cjson] = "--enable-cjson,--disable-cjson"

EXTRA_OECONF += "${@bb.utils.contains('PACKAGECONFIG', 'cjson', '--enable-cjson', '--disable-cjson', d)}"
RDEPENDS:${PN} += "${@bb.utils.contains('PACKAGECONFIG', 'cjson', 'cjson', '', d)}"

Comment on lines +36 to +40
PACKAGECONFIG[cjson] = "--enable-cjson,--disable-cjson"

EXTRA_OECONF += "${@bb.utils.contains('PACKAGECONFIG', 'cjson', '--enable-cjson', '--disable-cjson', d)}"
RDEPENDS:${PN} += "${@bb.utils.contains('PACKAGECONFIG', 'cjson', 'cjson', '', d)}"

Comment on lines +475 to +496

log "Upload service started: interval=${UPLOAD_INTERVAL}s output=$OUTPUT_DIR staging=$STAGING_DIR hash=$last_hash"

while true; do
log "Sleeping ${UPLOAD_INTERVAL}s until next upload cycle..."
sleep "$UPLOAD_INTERVAL"

# ── Configstore change detection ──────────────────────────────────────
# Meminsight may have crashed and restarted with a new RUN_ID, a
# different OUTPUT_DIR, or a different interval since we last slept.
# Reload whenever the file content has changed.
current_hash="$(configstore_hash)"
if [ -n "$current_hash" ] && [ "$current_hash" != "$last_hash" ]; then
log "Configstore changed (hash $last_hash → $current_hash); reloading."
load_configstore
last_hash="$current_hash"
# Recompute derived values that depend on OUTPUT_DIR / UPLOAD_INTERVAL.
[ "$UPLOAD_INTERVAL" -eq 0 ] && UPLOAD_INTERVAL=900
STAGING_DIR="${OUTPUT_DIR%/}_staging"
log "Config reloaded: interval=${UPLOAD_INTERVAL}s output=$OUTPUT_DIR staging=$STAGING_DIR"
fi

Comment on lines +458 to +467
if ! $UPLOAD_ENABLED; then
log "Upload is not enabled in configstore; exiting."
cleanup_upload_trigger
exit 0
fi

if [ "$UPLOAD_INTERVAL" -eq 0 ]; then
UPLOAD_INTERVAL=900
log "No upload interval configured; defaulting to 900 seconds."
fi
Comment on lines +99 to +110
strip_dynamic_entries() {
local conf_file="$1"
local tmp_conf="/tmp/rdk_chrony.conf.$$"

ntpLog "Retrieve NTP Server URL from /lib/rdk/getPartnerProperty.sh..."
while [ "$attempts" -le "$max_attempts" ]; do
# Remove all server, pool, and makestep lines so every run starts from a
# clean slate — partner entries from a previous run are cleared along with
# build-time defaults, making a separate deduplication step unnecessary.
awk '!/^[[:space:]]*(server|pool|makestep)[[:space:]]/' \
"$conf_file" > "$tmp_conf"
cat "$tmp_conf" > "$conf_file"

ntpLog "Attempt $attempts/$max_attempts to retrieve NTP server URL(s)..."
get_ntp_hosts
rm -f "$tmp_conf"
Comment on lines 203 to 224
# Add NTP servers ("server" or "pool" directive) to the configuration file

for i in $(seq 0 4); do
host="${hosts[$i]}"
directive="${directives[$i]}"
raw="${all_settings[$i]}"
if [ -n "$host" ]; then
# use directive if set, else default to server
[ -z "$directive" ] && directive="server"
parse_settings "$raw"

# Only allow server or pool, default to server otherwise
if [ "$directive" != "server" ] && [ "$directive" != "pool" ]; then
directive="server"
fi
iburst_opt=""
[ "$s_iburst" = "true" ] && iburst_opt=" iburst"

if [ "$directive" = "pool" ]; then
printf "%s %s iburst minpoll %s maxpoll %s maxsources 4\n" "$directive" "$host" "$minPoll" "$maxPoll" >> "$CHRONY_CONF"
if [ "$s_type" = "pool" ]; then
maxsources_opt=""
[[ "$s_maxsources" =~ ^[1-9][0-9]*$ ]] && maxsources_opt=" maxsources $s_maxsources"
printf "pool %s%s%s minpoll %s maxpoll %s\n" \
"$host" "$maxsources_opt" "$iburst_opt" "$s_minpoll" "$s_maxpoll" >> "$CHRONY_CONF"
else
printf "%s %s iburst minpoll %s maxpoll %s\n" "$directive" "$host" "$minPoll" "$maxPoll" >> "$CHRONY_CONF"
printf "server %s%s minpoll %s maxpoll %s\n" \
"$host" "$iburst_opt" "$s_minpoll" "$s_maxpoll" >> "$CHRONY_CONF"
fi
conf_written=1
fi
done
Copilot AI review requested due to automatic review settings May 26, 2026 09:44
Copy link
Copy Markdown
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

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Comment on lines +24 to +25
Type=oneshot
RemainAfterExit=yes
[Service]
Type=oneshot
Type=simple
RemainAfterExit=yes
Copilot AI review requested due to automatic review settings May 28, 2026 00:24
Copy link
Copy Markdown
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

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

@github-advanced-security
Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Copilot AI review requested due to automatic review settings June 2, 2026 14:41
Copy link
Copy Markdown
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

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

Comment on lines +103 to +109
acquire_lock() {
if ! mkdir "$LOCK_DIR" >/dev/null 2>&1; then
log "Another upload instance is already running; exiting."
exit 0
fi
trap cleanup_lock EXIT INT TERM
}
Comment on lines +464 to +467
if [ "$UPLOAD_INTERVAL" -eq 0 ]; then
UPLOAD_INTERVAL=900
log "No upload interval configured; defaulting to 900 seconds."
fi
Comment on lines +491 to +494
# Recompute derived values that depend on OUTPUT_DIR / UPLOAD_INTERVAL.
[ "$UPLOAD_INTERVAL" -eq 0 ] && UPLOAD_INTERVAL=900
STAGING_DIR="${OUTPUT_DIR%/}_staging"
log "Config reloaded: interval=${UPLOAD_INTERVAL}s output=$OUTPUT_DIR staging=$STAGING_DIR"
Comment on lines +399 to +401
if ! (cd "$OUTPUT_DIR" && run_low_priority tar -czf "$archive_path" $rel_files); then
log "tar failed; removing partial archive. Source reports untouched in $OUTPUT_DIR."
rm -f "$archive_path"
Comment on lines +23 to +26
[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=/lib/rdk/upload_MemReports.sh
Comment on lines +24 to 26
Type=simple
RemainAfterExit=yes
EnvironmentFile=-/tmp/meminsight.env
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.