sensors: fix CSV selection and prevent false passes by smuppand · Pull Request #616 · qualcomm-linux/qcom-linux-testkit

Srikanth Muppandam (smuppand) · 2026-10-12T15:55:26Z

Summary

Fix false Sensors results when explicit CSV selections contain unavailable sensor types.

The quoted set -- "$SENSORS_CSV" kept the whole CSV as one sensor name. With --strict 0, this emptied the target set and incorrectly produced a control-plane PASS without running any selected sensor. The supplied DUT log reproduced this with accel,gravity,gyro,mag,pressur, even though the correctly named individual sensors were listed in inventory.

Parse CSV names individually, trim surrounding whitespace, deduplicate while preserving order and reject empty/unsafe entries before runtime operations.
Record unavailable selections as FAIL in strict mode or SKIP with --strict 0. An empty automatic/non-strict streaming selection is SKIP, never PASS.
Use shared result initialization, counters and finalization. Missing optional driver-tool coverage is recorded as SKIP rather than counted as PASS.
Require successful see_workhorse exit and its final documented PASS marker. Arbitrary nonempty output cannot establish success.
Preserve timeout failure even when a client exits zero after the deadline, without changing the existing timeout duration or signal sequence.
Treat failed discovery or missing TYPE output as FAIL, retain actionable diagnostics and document the policy.

Compatibility

Existing sensor inventory, auto/named-profile selection, CSV-over-profile precedence, optional ssc_drva_test support, package/provider flow, service handling and YAML interface are retained. Explicit --list still reports inventory-query success and states that streaming was not validated. No board or distro-specific selection rules are introduced.

With --strict 0, a partially available list can pass only after available sensors actually validate, with unavailable entries separately recorded as SKIP. Strict mode fails missing selections before streaming starts.

Add a pure CSV parser that trims and deduplicates TYPE names while rejecting empty or unsafe fields. Require successful see_workhorse exit and its final PASS marker, and retain timeout failure even when the client exits zero. Keep existing helper call signatures and execution timing. This foundation is usable with the existing runner and supplies sensors_parse_csv for the following integration commit. Signed-off-by: Srikanth Muppandam <smuppand@qti.qualcomm.com>

Use shared CSV parsing and result orchestration. Classify unavailable selections according to strict mode, skip empty streaming selections and keep optional driver coverage visible without counting it as a pass. Validate selection input before runtime operations and document the operator policy. Depends on the shared sensors helpers in 9126ee2. Revert this consumer commit before reverting the foundation, or revert both together. Signed-off-by: Srikanth Muppandam <smuppand@qti.qualcomm.com>

abbajaj806 · 2026-10-13T09:36:17Z

-# shellcheck disable=SC2034
+# shellcheck disable=SC1090
+. "$INIT_ENV"
+# shellcheck disable=SC1091


This seems to be duplicate
We are already initializing it above

Yes, there are two source statements in this bootstrap. The second does not repeat initialization: Runner/init_env lines 5-7 return immediately when __INIT_ENV_LOADED is set. The runner block retains the prescribed bootstrap pattern, including that guarded second source. It is redundant at the call site, but does not execute setup twice.

abbajaj806 · 2026-10-13T09:37:11Z

@@ -123,9 +200,10 @@ else
 fi



There is no if for this fi

The opening if is present just above the displayed diff context. Lines 194-200 contain the complete if / elif / else / fi block for resolving and entering the test directory. The fi at line 200 closes the if at line 194. I also reran dash -n and ShellCheck 0.9.0 on the current file, both passed.

abbajaj806 · 2026-10-13T09:41:33Z

@@ -183,9 +267,7 @@ if [ "$OVERLAY_REQUESTED" -eq 1 ]; then
      pkg_ensure_optional_package_set_present \
      pkg_verify_package_set_installed; do
      if ! command -v "$required_helper" >/dev/null 2>&1; then


This fragment has no surrounding loop:

IMO
It should be something like this

for required_helper in
pkg_ensure_optional_package_set_present
pkg_verify_package_set_installed
do
if ! command -v "$required_helper" >/dev/null 2>&1; then
test_result_finish FAIL
"Required package helper is unavailable: $required_helper"
fi
done

The surrounding loop is already present in the full file at lines 265-272. It iterates over pkg_provider_init, pkg_ensure_optional_package_set_present and pkg_verify_package_set_installed, then checks each helper with command -v. The diff excerpt starts partway through that list and omits the opening for required_helper in.

abbajaj806 · 2026-10-13T09:43:59Z

+        test_result_finish FAIL "Failed to start $SENSORS_SERVICE, check the service status"
      fi
    fi



The shown code contains else and fi without the corresponding opening if statements. A structurally valid version would resemble:

The matching opening conditions are present in the full service block, lines 327-343. The outer condition checks distro support and systemctl, the next checks that the unit exists, and the inner conditions check whether it is active and whether starting it fails. The else at line 340 belongs to the unit-existence check at line 329. The diff omits some unchanged context. Both dash -n and ShellCheck pass on this revision.

abbajaj806 · 2026-10-13T10:16:23Z

+  TARGET_NL="$REQUESTED_TARGETS"
 else
  # 2) profile selection
  case "$PROFILE" in


The profile selection is incomplete
The current block has no profile labels:

The profile labels are present immediately below the displayed excerpt: lines 396-425. The first case handles automatic discovery, and the second handles basic|core, vision and all, with a no-op auto arm and an invalid-profile fallback. No profile labels are missing. Combining the two case blocks could improve readability, but is not required to restore profile coverage.

abbajaj806 · 2026-10-13T10:17:12Z

  if [ "$rc" -eq 0 ]; then
-    log_info "ssc_drva_test: PASS ($s) [log: $OUT_DIR/ssc_drva_test_${s}.log]"
-    pass_count=$((pass_count + 1))
+    test_result_record PASS "ssc_drva_test ($s), log=$OUT_DIR/ssc_drva_test_${s}.log"


The script uses SEE_DURATION, DRVA_DURATION, but their defaults are not shown:

Both defaults are already set before execution at lines 209-212: SEE_DURATION="${SEE_DURATION:-5}" and DRVA_DURATION="${DRVA_DURATION:-10}". These preserve nonempty CLI/environment overrides and otherwise use 5 and 10 seconds respectively. They are unchanged lines outside this diff excerpt, and the same defaults are documented in --help.

abbajaj806

LGTM

Srikanth Muppandam (smuppand) force-pushed the sensors branch 2 times, most recently from 403c9e5 to dc7e857 Compare October 12, 2026 16:50

Srikanth Muppandam (smuppand) requested a review from Bhargav-qcom (bhargav0610) October 12, 2026 16:52

Srikanth Muppandam (smuppand) added 2 commits October 12, 2026 22:25

Srikanth Muppandam (smuppand) force-pushed the sensors branch from dc7e857 to d9a7277 Compare October 12, 2026 16:55

abbajaj806 reviewed Oct 13, 2026

View reviewed changes

Srikanth Muppandam (smuppand) requested a review from abbajaj806 October 14, 2026 01:02

abbajaj806 approved these changes Oct 14, 2026

View reviewed changes

abbajaj806 merged commit 0bf20a2 into qualcomm-linux:main Oct 14, 2026
14 checks passed

Bhargav-qcom (bhargav0610) approved these changes Oct 14, 2026

View reviewed changes

Conversation

Srikanth Muppandam (smuppand) commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Compatibility

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abbajaj806 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Srikanth Muppandam (smuppand) commented Oct 12, 2026 •

edited

Loading