DEV-818 Add BMP581 pressure sensor support hooks by s-varna · Pull Request #111 · ShimmerResearch/log-and-stream-common

s-varna · 2026-07-07T13:31:39Z

Summary

Add ShimBrd_isBmp581PresentPerSrNumber() to identify BMP581-fitted boards by SR number (SR31>=11.2, SR47>=8.2, SR48>=8.2, SR49>=4.2)
Add PRESSURE_SENSOR_BMP581 = 3 enum value and NACK GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND for BMP581 (no calibration coefficients -- outputs pre-compensated data)
Widen OSR clamp from BMP3_OVERSAMPLING_32X to BMP5_OVERSAMPLING_128X when BMP581 is in use
Branch SD header calibration write and SD config file key (pres_bmp581_prec vs pres_bmp390_prec) on sensor type

Test plan

Verify BMP581 boards are correctly identified by SR number via ShimBrd_isBmp581PresentPerSrNumber()
Verify GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND returns NACK (0xFE) on BMP581 units
Verify OSR values 0-7 (up to 128x) accepted on BMP581 units, 0-5 max on BMP390
Verify SD header calibration block is skipped for BMP581
Verify SD config file uses pres_bmp581_prec key on BMP581 units and parses both keys on read

- ShimBrd_isBoardSrNumberGte() + ShimBrd_isBmp581PresentPerSrNumber() to identify up-rev'd Shimmer3R boards fitted with a BMP581 (>= SR31-11-2, SR47-8-2, SR48-8-2, SR49-4-2) - Add PRESSURE_SENSOR_BMP581 identity - NACK GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND when a BMP581 is fitted (self-compensating, no calibration coefficients), in keeping with the legacy BMP180/BMP280 command approach - Skip the 21-byte SD-header calibration block for the BMP581 - Widen the pressure oversampling clamp 0-5 -> 0-7 on BMP581 (64x/128x) - New SD cfg key pres_bmp581_prec (pres_bmp390_prec still parsed) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

s-varna · 2026-07-07T14:03:59Z

@copilot review

Copilot · 2026-07-07T14:08:44Z

@copilot review

Found one real bug and fixed it (commit Fix ShimBrd_isBoardSrNumberGte):

ShimBrd_isBoardSrNumberGte — incorrect version comparison (Boards/shimmer_boards.c)

The original major >= M && minor >= m condition is not equivalent to a version-ordered >=. For example, board revision 12.0 should satisfy >= 11.2, but 12 >= 11 && 0 >= 2 evaluates to false, so any board whose major version exceeds the BMP581 threshold minor would silently miss detection.

Fixed to the standard two-part comparison:

(brd_major > exp_brd_major) || (brd_major == exp_brd_major && brd_minor >= exp_brd_minor)

Everything else — NACK for GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND, OSR clamp widening, SD header skip, and SD config key branching — looks correct. CodeQL and automated code review both came back clean.

Current Consensys aborts its connect-time handshake when it receives a NACK on GET_PRESSURE_CALIBRATION_COEFFICIENTS, so a BMP581-fitted unit cannot connect. Until the host handles the NACK / the BMP581 sensor id, respond normally and report PRESSURE_SENSOR_BMP390 so the connection completes. Marked with TODO DEV-818 to restore the intended BMP581 NACK once the host side is ready. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The BMP581 self-compensates and has no calibration coefficients, so GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND now NACKs when a BMP581 is fitted (BMP390 path unchanged). The host must treat the NACK as "pre-compensated - convert raw pressure/temperature directly". Note: current Consensys aborts its connect handshake on this NACK; this is intended for NACK-aware hosts/test scripts. Add bmp581.py: streams BMP581 pressure/temperature over Bluetooth, expects the NACK, and converts straight from raw values (Pa = raw/64, degC = signed(raw)/65536). Accepts a COM port or a MAC (resolves MAC -> outgoing COM port) with an interactive port picker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

s-varna · 2026-07-15T15:19:34Z

@copilot review

…in response builder

…ys NACKs this path

Copilot · 2026-07-15T15:23:38Z

@copilot review

Found one stale comment with a real side-effect: the TODO DEV-818 in the GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND response builder (ShimBt_sendRsp) still said the NACK was "temporarily disabled" and hardcoded PRESSURE_SENSOR_BMP390 for all SHIMMER3R units. Since the NACK was re-enabled in the latest commit, BMP581 units never reach that code path, but the comment was misleading. Fixed in two commits:

Initially updated the comment and added an isBmp581InUse() conditional defensively.
On code-review feedback, removed the dead conditional (the NACK in ShimBt_processCmd() genuinely prevents BMP581 from reaching the response builder) and left PRESSURE_SENSOR_BMP390 with a clear comment explaining why. CodeQL and code review are both clean.

s-varna

all look fine

Copilot

Pull request overview

Adds BMP581 pressure sensor support hooks across the Shimmer3R firmware stack (board identification, BT command behavior, oversampling/config persistence), plus a dedicated Bluetooth streaming test script for BMP581.

Changes:

Adds board SR-number helper + BMP581-by-SR detection (ShimBrd_isBoardSrNumberGte, ShimBrd_isBmp581PresentPerSrNumber).
Updates pressure calibration/OSR handling for BMP581 (NACK calibration-coeffs command; widen OSR clamp for BMP581).
Branches SD header calibration write and SD cfg key (pres_bmp581_prec vs pres_bmp390_prec) by sensor type; adds BMP581 BT test script.

Reviewed changes

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

Show a summary per file

File	Description
SDCard/shimmer_sd_header.c	Skips writing BMP calibration block into SD header when BMP581 is in use.
SDCard/shimmer_sd_cfg_file.c	Writes/reads a BMP581-specific pressure OSR key while remaining backward-compatible on parsing.
Extras/python_scripts/Bluetooth commands/bmp581.py	New BMP581 BT streaming verification script (expects NACK on calibration-coeffs command).
Configuration/shimmer_config.c	Adjusts OSR input clamping to allow BMP581’s higher oversampling options.
Comms/shimmer_bt_uart.h	Extends pressure sensor enum with `PRESSURE_SENSOR_BMP581`.
Comms/shimmer_bt_uart.c	NACKs `GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND` when BMP581 is fitted; clarifies send path assumptions.
Boards/shimmer_boards.h	Declares new SR comparison helper + BMP581-by-SR detection API.
Boards/shimmer_boards.c	Implements SR `>=` comparison and SR-number based BMP581 presence detection.

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

…PR review) - Use the canonical `# -*- coding: utf-8 -*-` cookie so the UTF-8 declaration is unambiguously recognised. - Guard the calibration-response length-byte read against an empty (timed-out) read so a transient link/timeout prints a warning and returns cleanly instead of raising IndexError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…GSR 7.2 - GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND: move the BMP581 NACK out of ShimBt_processCmd and into ShimBt_sendRsp, alongside the BMP180/BMP280 handling, per review. processCmd now just routes the command; the BMP581 case in sendRsp emits a clean NACK (resets packet_length and writes only the NACK byte, since the ACK/NACK bytes are written before the response switch). - ShimBrd_isBmp581PresentPerSrNumber(): lower EXP_BRD_GSR_UNIFIED threshold from 8.2 to 7.2 per review (GSR-unified boards carry the BMP581 from 7.2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Mirror the DEV-818 board list 1:1 in ShimBrd_isBmp581PresentPerSrNumber(): SR31-11-2, SR47-8-2, SR48-7-2, SR48-8-2, SR49-4-2. SR48 (GSR unified) is listed for both the 7.x and 8.x lines, so keep both GSR checks (7.2 added per review alongside the existing 8.2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The SR48 rev-7 check was >= 7.2, which also matched SR48-8-0 and SR48-8-1 (major > 7) - boards that do NOT carry the BMP581. Bound the rev-7 case to major == 7 && minor >= 2 (>= 7.2 AND < 8.0); rev 8 stays covered by the separate >= 8.2 check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Revert the relocation into ShimBt_sendRsp. In sendRsp the ACK/NACK byte is written before the response switch with no re-check, so setting sendNack from inside the switch (as the BMP180/BMP280 cases do) yields an ACK with no data and leaves sendNack set for the next command. This command is sent at connect, so it must return a clean NACK - decide it in ShimBt_processCmd where sendAck vs sendNack is actually chosen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

ShimBt_sendRsp wrote the ACK/NACK byte before the response switch with no re-check, so a case setting sendNack (GET_BMP180/BMP280 on SHIMMER3R) sent an ACK with no data and left sendNack set for the next command. Add a post-switch fixup: if a case set sendNack, overwrite resPacket[0] with NACK and truncate to one byte -> clean 0xFE. No-op for every existing path (ACK-then-data, processCmd-set NACKs) and for the SHIMMER3 build (no switch case sets sendNack there, so classic BMP180/280 behaviour is unchanged). With the mechanism fixed, handle the BMP581 pressure-calibration NACK in the GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND case in ShimBt_sendRsp (next to BMP180/280) so all three are handled consistently in one place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

bmp581_plot.py: live raw + host temp-corrected pressure (3 panels), CSV log, per-unit quadratic/cubic correction + 2-point temp calibration + offset. bmp390_plot.py: host BMP3-compensated reference with the same acquisition path. bmp_compare.py: combined 5-panel BMP581-vs-BMP390 figure + slope/offset stats. All resilient to BT dropouts (stall watchdog + auto-reconnect). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

s-varna and others added 2 commits July 7, 2026 14:13

Committing clang-format changes

826f9b1

Copilot started work on behalf of s-varna July 7, 2026 14:04 View session

Fix ShimBrd_isBoardSrNumberGte: use correct two-part version comparison

69e1ac2

Copilot finished work on behalf of s-varna July 7, 2026 14:09

s-varna and others added 2 commits July 9, 2026 17:40

Copilot started work on behalf of s-varna July 15, 2026 15:19 View session

Copilot AI added 2 commits July 15, 2026 15:21

Fix stale TODO DEV-818 comment and add PRESSURE_SENSOR_BMP581 branch …

0c8c3a1

…in response builder

Remove redundant isBmp581InUse check in response builder; BMP581 alwa…

6423406

…ys NACKs this path

Copilot finished work on behalf of s-varna July 15, 2026 15:24

s-varna commented Jul 16, 2026

View reviewed changes

s-varna requested review from Copilot and marknolan July 16, 2026 12:11

Copilot started reviewing on behalf of s-varna July 16, 2026 12:12 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread Extras/python_scripts/Bluetooth commands/bmp581.py Outdated

Comment thread Extras/python_scripts/Bluetooth commands/bmp581.py

marknolan reviewed Jul 16, 2026

View reviewed changes

Comment thread Boards/shimmer_boards.c

marknolan reviewed Jul 16, 2026

View reviewed changes

Comment thread Comms/shimmer_bt_uart.c

marknolan requested changes Jul 16, 2026

View reviewed changes

s-varna and others added 6 commits July 16, 2026 13:39

Committing clang-format changes

45492c7

marknolan approved these changes Jul 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DEV-818 Add BMP581 pressure sensor support hooks - #111

DEV-818 Add BMP581 pressure sensor support hooks#111
s-varna wants to merge 15 commits into
mainfrom
DEV-818_BMP581_pressure_sensor_support

s-varna commented Jul 7, 2026

Uh oh!

s-varna commented Jul 7, 2026

Uh oh!

Copilot AI commented Jul 7, 2026

Uh oh!

s-varna commented Jul 15, 2026

Uh oh!

Copilot AI commented Jul 15, 2026

Uh oh!

s-varna left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

s-varna commented Jul 7, 2026

Summary

Test plan

Uh oh!

s-varna commented Jul 7, 2026

Uh oh!

Copilot AI commented Jul 7, 2026

Uh oh!

s-varna commented Jul 15, 2026

Uh oh!

Copilot AI commented Jul 15, 2026

Uh oh!

s-varna left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants