fixed some edge cases and improved the commenting

[reubxn]

Commented the code better to improve readability. Also:

Fixed MPU6050 polling in the main loop:
Swapped out the while catch-up loop for a safer if statement. Previously, if the CPU fell slightly behind schedule (like during a heavy Bluetooth transmission), it would rapid-fire stale sensor measurements to "catch up" the timer. This flooded our low-pass filter and skewed the RMS shake scores. Now, the system safely skipping missed samples and just waits for the next valid reading interval.

Moved BLE advertising out of the background:
Calling BLEDevice::startAdvertising() directly inside the onDisconnect event callback is risky because it runs on a background task. This was occasionally triggering FreeRTOS Watchdog panics and crashing the ESP32. We removed the direct call and instead flip a g_shouldAdvertise flag, allowing the main loop() to safely restart the radio on the main application thread.

Fixed Si7021 loose-wire checks:
The code that attempts to cleanly reconnect the Si7021 temperature sensor every 2 seconds was accidentally nested inside the !hasMpu block. This meant our hot-plug safety net was completely ignored as long as the motion sensor was fully connected. We untangled this by moving the Si7021 fallback checks to run independently, so the system now properly recovers the thermometer if a user bumps a wire.

1. 📝 Description

• Improves robustness and clarity:
  • Skips stale MPU6050 samples when lagging to avoid filter flooding and skewed RMS.
  • Defers BLE advertising restart to main loop via g_shouldAdvertise to prevent ESP32 WDT panics.
  • Decouples Si7021 reconnect from MPU presence so hot-plug can recover anytime.
  • Refines time-sync critical sections and clarifies packet/comment docs.

2. 📓 References

• ESP-IDF Watchdog Timer (ESP32): https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/system/wdts.html
• NimBLE-Arduino (ESP32 BLE stack): https://github.com/h2zero/NimBLE-Arduino
• MPU-6000/MPU-6050 Register Map: https://invensense.tdk.com/wp-content/uploads/2015/02/MPU-6000-Register-Map1.pdf
• Si7021-A20 Humidity/Temperature Sensor Datasheet: https://www.silabs.com/documents/public/data-sheets/Si7021-A20.pdf

3. 📦 Dependencies & Requirements

• No new dependencies or version changes.
• No environment variable or configuration changes.
• Public APIs/packet layouts unchanged (comments clarified only).

4. 📊 Contributor Summary
   | Contributor | Lines Added | Lines Removed | Files Changed |
   | reubxn | 110 | 103 | 1 |

5. 🗂️ Changes Summary
   | File (relative path) | Lines Added | Lines Removed |
   | Code/Sensors/Sensy/MYOSA_VibrationLogger_ESP32/firmware/MYOSA_VibrationLogger/MYOSA_VibrationLogger.ino | 110 | 103 |

[coderabbitai]

📝 Walkthrough

Walkthrough

Updated an ESP32 vibration logger firmware with improved documentation clarity, modified BLE reconnection behavior via a new flag, enhanced time-synchronization concurrency handling, refined sensor sampling from a catch-up loop to a pacemaker model, and expanded sensor initialization retry logic.

Changes

Cohort / File(s)	Summary
Documentation & Comments MYOSA_VibrationLogger.ino	Updated in-file labels and documentation describing vibration computation (RMS/peak), time-sync behavior, and BLE packet field meanings (timestamp and sensor unit descriptions); no struct or functional logic changes.
BLE Reconnection & Advertising MYOSA_VibrationLogger.ino	Added global g_shouldAdvertise flag; on disconnect, firmware now sets the flag instead of immediately calling BLEDevice::startAdvertising(), and loop() conditionally restarts advertising when flag is set.
Time Synchronization & Concurrency MYOSA_VibrationLogger.ino	Enhanced getEpochMsOrZero() with explicit critical section around epoch base variables; strengthened documentation of concurrent access guards.
Sensor Sampling Timing MYOSA_VibrationLogger.ino	Replaced catch-up sampling loop with single-step pacemaker condition taking at most one sample per samplePeriodUs; added lag-detection logic to adjust lastSampleUs when exceeding 2× samplePeriodUs.
Sensor Initialization & Retry Logic MYOSA_VibrationLogger.ino	Expanded retry behavior to run every 2s and attempt re-initialization of both MPU6050 and Si7021 based on individual sensor availability flags instead of MPU-only retry.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'fixed some edge cases and improved the commenting' is vague and generic, using non-descriptive terms like 'edge cases' and 'some' that don't convey what specific issues were fixed or the main technical changes.	Consider revising the title to specifically highlight one main change such as 'Replace catch-up sampling with pacemaker loop to prevent filter skew' or 'Fix BLE advertising reconnect race condition with flag-based approach'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch reuban-review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Code/Sensors/Sensy/MYOSA_VibrationLogger_ESP32/firmware/MYOSA_VibrationLogger/MYOSA_VibrationLogger.ino (1)

546-551: ⚠️ Potential issue | 🟡 Minor

Potential I2C bus contention when Si7021 is absent but MPU6050 is healthy.

Per the si7021_min.cpp implementation, si7021.begin() performs a soft reset and user register read via I2C transactions even when the sensor is absent. When the MPU6050 is connected but Si7021 is not, these failed I2C transactions every 2 seconds could leave the shared I2C bus in an undefined state, potentially causing intermittent MPU6050 read failures.

Consider adding a brief I2C bus recovery (e.g., toggle SCL a few times) after a failed Si7021 init, or limit retry attempts before backing off longer.

🛡️ Suggested mitigation

       if (!hasSi7021) {
         if (si7021.begin(Wire, 0x40)) {
           hasSi7021 = true;
           Serial.println("[Init] Si7021 found (using ambient temperature)");
+        } else {
+          // Clear any I2C bus stuck condition after failed probe
+          Wire.beginTransmission(0x00);
+          Wire.endTransmission(true);
         }
       }

Alternatively, consider exponential backoff for Si7021 retries when MPU6050 is healthy to reduce bus interference.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@Code/Sensors/Sensy/MYOSA_VibrationLogger_ESP32/firmware/MYOSA_VibrationLogger/MYOSA_VibrationLogger.ino`
around lines 546 - 551, The Si7021 init path (si7021.begin(Wire, 0x40) setting
hasSi7021) can leave the shared I2C bus in a bad state when the sensor is
absent; after a failed si7021.begin() call add a short I2C recovery sequence
(toggle SCL a few times via the Wire/SDA/SCL GPIOs) or call
Wire.end()/Wire.begin() to reset the bus, and implement exponential
backoff/limited retry for subsequent si7021.begin() attempts; tie retries to the
MPU6050 health check so when the MPU6050 is healthy you back off longer to avoid
repeated bus contention. Ensure you update the logic around hasSi7021 and the
code that calls si7021.begin(...) to perform the recovery and backoff behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@Code/Sensors/Sensy/MYOSA_VibrationLogger_ESP32/firmware/MYOSA_VibrationLogger/MYOSA_VibrationLogger.ino`:
- Around line 546-551: The Si7021 init path (si7021.begin(Wire, 0x40) setting
hasSi7021) can leave the shared I2C bus in a bad state when the sensor is
absent; after a failed si7021.begin() call add a short I2C recovery sequence
(toggle SCL a few times via the Wire/SDA/SCL GPIOs) or call
Wire.end()/Wire.begin() to reset the bus, and implement exponential
backoff/limited retry for subsequent si7021.begin() attempts; tie retries to the
MPU6050 health check so when the MPU6050 is healthy you back off longer to avoid
repeated bus contention. Ensure you update the logic around hasSi7021 and the
code that calls si7021.begin(...) to perform the recovery and backoff behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab29cf9e-d1c8-4b41-9e15-fadb792640e6

📥 Commits

Reviewing files that changed from the base of the PR and between 0945df1 and 8b03957.

📒 Files selected for processing (1)

• Code/Sensors/Sensy/MYOSA_VibrationLogger_ESP32/firmware/MYOSA_VibrationLogger/MYOSA_VibrationLogger.ino