Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces comprehensive firmware development for motor and sensor microcontrollers with microROS integration, GitHub Actions CI/CD workflows for automated testing, and ROS interface message definitions alongside frontend type bindings for cross-system communication. Changes
Sequence Diagram(s)sequenceDiagram
participant SensorMCU as Sensor MCU<br/>(RP2040)
participant ROS as ROS Agent<br/>(Host)
participant MotorMCU as Motor MCU<br/>(RP2040)
participant Frontend as Frontend<br/>(React)
rect rgba(100, 150, 200, 0.5)
Note over SensorMCU,Frontend: microROS Initialization
SensorMCU->>ROS: create_entities()
ROS-->>SensorMCU: entity creation confirmation
MotorMCU->>ROS: create_entities()
ROS-->>MotorMCU: entity creation confirmation
end
rect rgba(100, 150, 200, 0.5)
Note over SensorMCU,Frontend: Main Control Loop - Sensor Data Flow
SensorMCU->>SensorMCU: Read sensors (ADC, IMU, PIR, etc.)
SensorMCU->>SensorMCU: Calculate omegaRef & thetaRef
SensorMCU->>ROS: transmitMsg(thetaRef, omegaRef,<br/>ultrasonic, pir, fan, imu)
ROS->>Frontend: Publish sensor data
Frontend->>Frontend: Update UI with sensor values
end
rect rgba(150, 100, 200, 0.5)
Note over SensorMCU,Frontend: Motor Control Flow
ROS->>MotorMCU: Subscription: RefSpeed message
MotorMCU->>MotorMCU: getRefSpeed()
MotorMCU->>MotorMCU: Calculate DAC values
MotorMCU->>MotorMCU: Set motor direction/brake/enable
MotorMCU->>ROS: transmitDac(leftDAC, rightDAC)
MotorMCU->>ROS: Publish battery & motor speeds
ROS->>Frontend: Display battery & motor feedback
end
rect rgba(200, 150, 100, 0.5)
Note over SensorMCU,Frontend: Actuator Control (Subscriptions)
Frontend->>ROS: User commands (light, lidar, fan)
ROS->>SensorMCU: Subscription callbacks
SensorMCU->>SensorMCU: light_subscription_callback()
SensorMCU->>SensorMCU: lidar_subscription_callback()
SensorMCU->>SensorMCU: fan_subscription_callback()
SensorMCU->>SensorMCU: Apply control changes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The review spans extensive new firmware across two microcontroller platforms with diverse sensor integrations, microROS communication setup involving state machines and lifecycle management, ROS message interface definitions, and frontend type bindings. While individual sensor modules follow consistent patterns, the breadth of affected areas, interaction complexity between microcontrollers and ROS, and critical control loop logic (motor speed calculation, battery management, actuator commands) demand comprehensive review effort. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
firmware/sensor_microcontroller/README.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 MajorCI badge points to the old repository — will display incorrectly or always fail.
The badge URL references
WheelchairSeniorDesign/SensorMicrocontroller, the previous standalone repo. Now that this code lives inTourGuideSeniorDesign/tour-guide-code-module, the badge should point to the new monorepo's workflow.📝 Proposed fix
-[](https://github.com/WheelchairSeniorDesign/SensorMicrocontroller/actions/workflows/Platformio_CI.yml) +[](https://github.com/TourGuideSeniorDesign/tour-guide-code-module/actions/workflows/Platformio_CI.yml)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/README.md` at line 1, Update the CI badge markdown in README.md so it points to the new monorepo: replace the repository path WheelchairSeniorDesign/SensorMicrocontroller with TourGuideSeniorDesign/tour-guide-code-module in the badge URL and link (the badge line containing Platformio_CI.yml), ensuring both the image src and the href reference the new repo/workflow path.firmware/sensor_microcontroller/src/PWMFunctions.cpp-15-17 (1)
15-17:⚠️ Potential issue | 🟠 MajorClock divider required for frequencies below ~1.9 kHz; current code will silently produce incorrect PWM output.
The fixed 125 MHz clock with a 16-bit wrap register limits frequencies to
f = 125,000,000 / (wrap + 1), where wrap ≤ 65535. This yields a minimum frequency of ~1,907 Hz without additional division. The current code does not callpwm_set_clkdiv(), so any request below 1.9 kHz will overflow the 16-bit register and produce silent incorrect output (e.g., 1000 Hz request truncates wrap to ~59463, generating ~2.1 kHz instead).Either validate input frequencies and reject values below the minimum, or implement
pwm_set_clkdiv()to extend the range down to ~7.5 Hz (with divider ≈ 256).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/PWMFunctions.cpp` around lines 15 - 17, The code currently assumes a fixed clock_freq and directly computes wrap causing 16-bit overflow for frequencies below ~1.9 kHz; update the PWMFunctions.cpp logic around clock_freq, wrap, and pwm_set_wrap to compute and set an appropriate clock divider via pwm_set_clkdiv(slice, clkdiv) when needed: compute desired_wrap = clock_freq / (frequency * clkdiv) - 1, choose the smallest clkdiv (float between 1.0 and 256.0) that yields desired_wrap <= 65535, clamp desired_wrap to uint16_t and call pwm_set_clkdiv(slice, clkdiv) then pwm_set_wrap(slice, wrap), and if no clkdiv in [1,256] produces a valid wrap, reject the requested frequency (return error) or clamp to the lowest supported frequency; reference the variables/function names clock_freq, wrap, pwm_set_wrap, pwm_set_clkdiv and ensure proper handling of types (float divider, uint16_t wrap) and edge cases.firmware/sensor_microcontroller/extra_packages/extra_packages.repos-1-5 (1)
1-5:⚠️ Potential issue | 🟠 MajorSame self-referential dependency and floating version as the motor counterpart
This file is identical to
firmware/motor_microcontroller/extra_packages/extra_packages.reposand has the same two problems: the dependency URL is the parent repo itself, andversion: mainis a non-pinned floating reference. See the comments on that file for details and the proposed fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/extra_packages/extra_packages.repos` around lines 1 - 5, The extra_packages.repos currently declares a self-referential repository entry "tour-guide-code-module" pointing to the parent repo URL and uses a floating "version: main"; replace the URL with the actual external submodule repo (do not point to the parent repository) and pin "version:" to a fixed tag or commit SHA (e.g., a release tag or specific commit) instead of "main" so the dependency is not floating and cannot create circular/self-dependency issues; update the "tour-guide-code-module" entry accordingly.firmware/motor_microcontroller/include/globals.h-8-9 (1)
8-9:⚠️ Potential issue | 🟠 MajorMissing
volatileon shared globals written from micro-ROS callbacks
speedRandspeedLare written in the micro-ROS subscription callback (microRosFunctions.cpp) and read in the main control loop. Withoutvolatile, the compiler is free to cache these values in registers and never re-read from memory, so the motor loop may never observe updated reference speeds.🔧 Proposed fix
-extern float speedR; -extern float speedL; +extern volatile float speedR; +extern volatile float speedL;Apply the same
volatilequalifier to the definitions in the corresponding.cppfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/motor_microcontroller/include/globals.h` around lines 8 - 9, Declare the shared globals speedR and speedL as volatile in globals.h and also mark their definitions in the corresponding .cpp file (the file where they are currently defined, e.g., globals.cpp) as volatile so updates made by the micro-ROS subscription callback in microRosFunctions.cpp are always read by the main control loop; update both the extern declarations (speedR, speedL) and the actual definitions to use the volatile qualifier to prevent compiler caching..github/workflows/sensor_microcontroller_CI.yml-46-46 (1)
46-46:⚠️ Potential issue | 🟠 Major
if-no-files-found: warnsilently passes CI when firmware is missingIf the build output path (
firmware/sensor_microcontroller/.pio/build/ROS/firmware.uf2) is wrong or the ROS build fails to produce the artifact, the workflow still completes green with just a warning. Set this toerrorso a missing artifact is a hard failure.- if-no-files-found: warn + if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/sensor_microcontroller_CI.yml at line 46, Change the artifact upload step to fail the run on missing firmware by replacing the actions/upload-artifact option if-no-files-found: warn with if-no-files-found: error for the step that uploads the path "firmware/sensor_microcontroller/.pio/build/ROS/firmware.uf2" (look for the upload-artifact step using that path) so a missing or misbuilt firmware causes the workflow to fail rather than pass with a warning..github/workflows/motor_microcontroller_CI.yml-46-46 (1)
46-46:⚠️ Potential issue | 🟠 Major
if-no-files-found: warnsilently passes CI when firmware is missingSame issue as the sensor CI workflow — a wrong path or failed ROS build still produces a green run. Set to
error.- if-no-files-found: warn + if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/motor_microcontroller_CI.yml at line 46, Change the workflow step that currently uses the runner option if-no-files-found: warn to if-no-files-found: error so missing firmware (or wrong paths/failed builds) fail the CI; locate the line with the YAML key if-no-files-found and replace its value from warn to error to ensure the job errors instead of passing silently.firmware/sensor_microcontroller/include/PIRFunctions.h-6-7 (1)
6-7:⚠️ Potential issue | 🟠 MajorMake the header self-contained for
uint8_tusage.At Line 26,
uint8_tis used without including a defining header in this file. Add<stdint.h>(or<Arduino.h>) here to avoid include-order-dependent compilation.Suggested fix
`#ifndef` PIRFUNCTIONS_H `#define` PIRFUNCTIONS_H + +#include <stdint.h>Also applies to: 26-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/PIRFunctions.h` around lines 6 - 7, PIRFunctions.h currently uses uint8_t (e.g., in function signatures or typedefs) without including a defining header; make the header self-contained by adding an include for <stdint.h> (or <Arduino.h>) at the top of PIRFunctions.h so uint8_t is always defined regardless of include order, ensuring symbols like uint8_t used in this file compile reliably.firmware/sensor_microcontroller/src/LightFunctions.cpp-24-30 (1)
24-30:⚠️ Potential issue | 🟠 MajorDefault branch should be fail-safe, not “light on”.
At Lines 27-29, invalid
lightStatevalues drive the same output as case 2. That can enable lights unexpectedly for out-of-range inputs. Add abreakfor case 2 and makedefaultexplicitly off.Suggested fix
case 2: digitalWrite(LIGHT_PIN_1, HIGH); digitalWrite(LIGHT_PIN_2, LOW); + break; default: - digitalWrite(LIGHT_PIN_1, HIGH); - digitalWrite(LIGHT_PIN_2, LOW); + digitalWrite(LIGHT_PIN_1, LOW); + digitalWrite(LIGHT_PIN_2, LOW); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/LightFunctions.cpp` around lines 24 - 30, The default branch is currently enabling lights for invalid lightState values; in the switch handling (case 2 / default) ensure case 2 ends with a break so it doesn't fall through, and change the default branch to a fail-safe that explicitly turns lights off by writing LOW to LIGHT_PIN_1 and LIGHT_PIN_2 instead of HIGH. Locate the switch in LightFunctions.cpp and update case 2 to include a break and set default to digitalWrite(LIGHT_PIN_1, LOW) and digitalWrite(LIGHT_PIN_2, LOW).firmware/sensor_microcontroller/include/UltrasonicFunctions.h-21-23 (1)
21-23:⚠️ Potential issue | 🟠 MajorReturn type conflicts with documented error sentinel.
At Line 21, docs specify
-1on misconfiguration, but Line 23 returnsuint16_t. That converts-1to65535, which breaks the stated API contract.Suggested fix
-uint16_t ultrasonicDistance(Adafruit_ADS1115 &adc, uint8_t pinNumber); +int16_t ultrasonicDistance(Adafruit_ADS1115 &adc, uint8_t pinNumber);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/UltrasonicFunctions.h` around lines 21 - 23, The function ultrasonicDistance currently returns uint16_t but the comment says it returns -1 on misconfiguration, causing a mismatch; change the return type to a signed integer (e.g., int32_t or int16_t) in the UltrasonicFunctions.h declaration and in the matching implementation (the ultrasonicDistance definition) and update all callers to handle the negative sentinel correctly; ensure the chosen signed type can represent all valid distances and -1 sentinel.firmware/motor_microcontroller/src/BatteryFuctions.cpp-6-7 (1)
6-7:⚠️ Potential issue | 🟠 MajorAvoid hardcoded ADC channel when
ADC_PINis configurable.At Line 7,
adc_select_input(2)can become incorrect ifADC_PINchanges. Derive channel fromADC_PINto keep pin/channel mapping consistent.Suggested fix
void initBatterySensor() { adc_init(); adc_gpio_init(ADC_PIN); - adc_select_input(2); + adc_select_input(ADC_PIN - 26); // RP2040 GPIO26..29 -> ADC0..3 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/motor_microcontroller/src/BatteryFuctions.cpp` around lines 6 - 7, The code hardcodes ADC channel 2 after calling adc_gpio_init(ADC_PIN); replace the fixed call to adc_select_input(2) with a derived channel from the configured pin by using the SDK helper that maps a GPIO to its ADC channel (e.g., adc_gpio_to_channel(ADC_PIN)), and pass that result to adc_select_input; also add a simple check for an invalid mapping (negative/invalid channel) and handle/log it before calling adc_select_input to avoid mis-selection when ADC_PIN changes.firmware/sensor_microcontroller/platformio.ini-62-63 (1)
62-63:⚠️ Potential issue | 🟠 MajorROS-Debug should also define
ROSto keep feature gating consistent.At Line 63,
ROS-Debugonly definesROS_DEBUG. If source files gate ROS code behind#ifdef ROS, this environment can silently diverge fromenv:ROS.Suggested fix
[env:ROS-Debug] ... build_flags = + -D ROS -D ROS_DEBUG🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/platformio.ini` around lines 62 - 63, The build flag currently only defines ROS_DEBUG (build_flags containing "-D ROS_DEBUG"); update the build_flags so ROS is also defined (add "-D ROS") to keep feature gating consistent with env:ROS; specifically modify the build_flags entry that contains "-D ROS_DEBUG" to include both "-D ROS" and "-D ROS_DEBUG" so source-level checks like `#ifdef` ROS behave the same across environments.firmware/sensor_microcontroller/include/IMUFunctions.h-30-30 (1)
30-30:⚠️ Potential issue | 🟠 Major
imuInitreturnstrueon failure — inverted convention.Looking at the implementation,
imuInitreturnstruewhen initialization fails (after 10 retries) andfalseon success. This is the opposite of the near-universal C/C++ convention wheretrue/non-zero means success. Call sites usingif (imuInit(...))expecting success will silently skip initialization errors.Consider inverting the return semantics or switching to an enum/error code for clarity.
Also minor: parameter names
accelRang/gyroRangappear to be typos foraccelRange/gyroRange.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/IMUFunctions.h` at line 30, The imuInit function currently returns true on failure and false on success; invert the return semantics so imuInit(Adafruit_ICM20948 &icm, icm20948_accel_range_t accelRange, icm20948_gyro_range_t gyroRange, ak09916_data_rate_t magDataRate) returns true on successful init and false on failure (or replace with an explicit enum/error code), update all call sites that check if (imuInit(...)) accordingly, and rename the parameters accelRang->accelRange and gyroRang->gyroRange in the declaration and implementation to fix the typos consistently.firmware/sensor_microcontroller/include/FanFunctions.h-47-53 (1)
47-53: 🛠️ Refactor suggestion | 🟠 MajorRemove
staticinterrupt handler declarations from the header.
staticfunctions in a header get internal linkage per translation unit — each.cppthat includes this header gets its own (unused) copy of these ISR stubs. Only the definitions inFanFunctions.cppare registered withattachInterrupt, so these declarations add no value and may trigger "unused function" warnings.Keep them solely in
FanFunctions.cpp(asstaticdefinitions or in an anonymous namespace).Suggested fix
-static void handleTach0Interrupt(); - -static void handleTach1Interrupt(); - -static void handleTach2Interrupt(); - -static void handleTach3Interrupt(); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/FanFunctions.h` around lines 47 - 53, Remove the four static ISR declarations (handleTach0Interrupt, handleTach1Interrupt, handleTach2Interrupt, handleTach3Interrupt) from the header so they no longer produce per-translation-unit internal-linkage stubs; keep the actual static definitions in FanFunctions.cpp (or move them into an anonymous namespace) where they are registered with attachInterrupt, ensuring only the real definitions exist and no unused static prototypes remain in the header.firmware/sensor_microcontroller/include/FingerprintFunctions.h-14-14 (1)
14-14:⚠️ Potential issue | 🟠 Major
loopFingerprint()silently drops the fingerprint ID — data is never published.The implementation in
firmware/sensor_microcontroller/src/FingerprintFunctions.cpp(lines 132–136) callsgetFingerprintID()but discards its return value, and never invokespublishFingerprint(). Any firmware main loop that relies solely onloopFingerprint()to drive fingerprint data onto the ROS graph will receive nothing.Either capture and publish the ID inside
loopFingerprint(), or document that callers must callgetFingerprintID()directly and pipe the result topublishFingerprint().🐛 Proposed fix in FingerprintFunctions.cpp
void loopFingerprint() { - getFingerprintID(); + uint8_t id = getFingerprintID(); + if (id != FINGERPRINT_NOFINGER && id != FINGERPRINT_NOTFOUND) { + publishFingerprint(id); + } delay(50); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/FingerprintFunctions.h` at line 14, loopFingerprint() currently calls getFingerprintID() and discards its result, so fingerprint data is never published; modify loopFingerprint() to store the return value of getFingerprintID() (e.g., int id = getFingerprintID()) and then call publishFingerprint(id) when a valid ID is obtained, ensuring any error/invalid return values from getFingerprintID() are handled (no publish). Alternatively, if you prefer not to change loopFingerprint(), update the header/docs to state clearly that callers must call getFingerprintID() and then pass that result to publishFingerprint() themselves; reference functions: loopFingerprint, getFingerprintID, publishFingerprint in FingerprintFunctions.cpp/h.firmware/sensor_microcontroller/include/FingerprintFunctions.h-12-12 (1)
12-12:⚠️ Potential issue | 🟠 Major
setupFingerprint()has an inverted return convention —truesignals failure.In
firmware/sensor_microcontroller/src/FingerprintFunctions.cpp(line ~104), the function returnstruewhen the sensor can't be found after 10 retries (the failure path), andreturn falseat the end of the function body (the success path). Any caller writingif (setupFingerprint()) { /* proceed */ }will proceed on failure and abort on success.🐛 Proposed fix in FingerprintFunctions.cpp
if (init_count > 10) { - return true; // currently: true = sensor not found (failure) + return false; // false = setup failed } init_count++; } } // ... (successful path at end of function) - return false; + return true; // true = setup succeeded🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/FingerprintFunctions.h` at line 12, The setupFingerprint() function currently uses inverted semantics (returns true on failure and false on success); change the implementation in FingerprintFunctions.cpp so that setupFingerprint() returns true on successful initialization and false on failure: flip the return at the retry-failure branch (where it currently returns true after 10 failed attempts) to return false, and change the final return false at the end of the function to return true; ensure any early-exit error returns also follow the same convention and update any callers if they relied on the old inverted behavior.src/autogiro_interfaces/msg/RefSpeed.msg-3-4 (1)
3-4:⚠️ Potential issue | 🟠 Major
lat_dispandlong_dispare silently discarded and not processed.The ROS message
RefSpeed.msgincludes these fields (lines 3-4), but the firmware'sgetRefSpeed()function (at lines 279-284 offirmware/motor_microcontroller/src/microRosFunctions.cpp) only extracts and mapsleft_speedandright_speedto the firmware struct. The displacement fields are received but never used, leading to data loss. Either remove these fields from the ROS message if they are unneeded, or add them to the firmware struct and implement proper handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/autogiro_interfaces/msg/RefSpeed.msg` around lines 3 - 4, The RefSpeed.msg fields lat_disp and long_disp are declared but never used by the firmware; update the firmware to consume them or remove them from the message. If keeping them, add matching fields (e.g., int8_t lat_disp; int8_t long_disp;) to the firmware RefSpeed struct/type and populate them in getRefSpeed() alongside left_speed and right_speed (convert ROS int8 to the firmware integer type), then propagate those fields to any downstream logic that needs lateral/longitudinal displacement; otherwise remove lat_disp and long_disp from RefSpeed.msg so the message and getRefSpeed() remain consistent.firmware/motor_microcontroller/src/main.cpp-220-240 (1)
220-240:⚠️ Potential issue | 🟠 Major
transmitDaccalled twice per loop whenROS_DEBUGis defined.When
ROS_DEBUGis defined,defined(ROS) || defined(ROS_DEBUG)at line 220 is true (callingtransmitDacat line 226), and#ifdef ROS_DEBUGat line 238 is also true (callingtransmitDacagain at line 239). This results in a double publish every loop iteration.🐛 Proposed fix — guard the second call
-#ifdef ROS_DEBUG - transmitDac(refSpeedL, refSpeedR); -#endifThe first
transmitDaccall on line 226 already coversROS_DEBUGvia thedefined(ROS) || defined(ROS_DEBUG)guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/motor_microcontroller/src/main.cpp` around lines 220 - 240, The code calls transmitDac(refSpeedL, refSpeedR) twice when ROS_DEBUG is defined due to both the `#if` defined(ROS) || defined(ROS_DEBUG) block and the later `#ifdef` ROS_DEBUG block; fix by ensuring only one call per loop: either remove the later transmitDac(...) in the `#ifdef` ROS_DEBUG block or change its guard to exclude ROS (e.g., `#ifdef` ROS_DEBUG && !defined(ROS)), so transmitDac is invoked exactly once; update the blocks surrounding transmitDac to use the transmitDac symbol and the ROS/ROS_DEBUG preprocessor flags accordingly.firmware/motor_microcontroller/src/main.cpp-85-94 (1)
85-94:⚠️ Potential issue | 🟠 MajorInfinite loop if DAC hardware is absent.
Both
while (!dacA.begin(...))andwhile (!dacB.begin(...))have no retry limit. If a DAC is disconnected or faulty, the board hangs permanently. Other init functions in this project (IMU, ADC, fingerprint) cap retries at ~10.🛡️ Proposed fix — add retry limit
+ int dacRetry = 0; - while (!dacA.begin(0x62)) + while (!dacA.begin(0x62) && dacRetry < 10) { Serial.println("DAC A not found"); delay(500); + dacRetry++; } + dacRetry = 0; - while (!dacB.begin(0x63)) + while (!dacB.begin(0x63) && dacRetry < 10) { Serial.println("DAC B not found"); delay(500); + dacRetry++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/motor_microcontroller/src/main.cpp` around lines 85 - 94, The infinite-blocking DAC init loops using dacA.begin(...) and dacB.begin(...) should be changed to bounded retry loops (matching other inits like IMU/ADC/fingerprint) — replace each while(!dacX.begin(addr)) with a for/attempt-count retry (e.g., max 10 attempts), call Serial.println on each failure as now, and after exceeding retries log a final error and proceed with a safe fallback (returning an error, setting a "dac_failed" flag, or continuing without blocking) so the MCU doesn't hang if the hardware is absent.firmware/sensor_microcontroller/src/UltrasonicFunctions.cpp-17-21 (1)
17-21:⚠️ Potential issue | 🟠 Major
return -1onuint16_tsilently wraps to 65535.Returning
-1from auint16_tfunction yields65535(UINT16_MAX) due to unsigned wrap-around. The caller cannot distinguish this from a valid (albeit large) distance reading. Either change the return type or use a sentinel that's clearly out of range, or return 0 and log the error.🐛 Proposed fix — return 0 for invalid pin
uint16_t ultrasonicDistance(Adafruit_ADS1115 &adc, uint8_t pinNumber){ if(pinNumber > 3){ Serial.println("Please select a pin between 0 and 3"); - return -1; + return 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/UltrasonicFunctions.cpp` around lines 17 - 21, The function ultrasonicDistance currently returns uint16_t but uses return -1 which wraps to 65535; change the signature of ultrasonicDistance(Adafruit_ADS1115 &adc, uint8_t pinNumber) to a signed integer type (e.g., int16_t or int32_t) so you can return -1 as an error sentinel, replace the return -1 with that sentinel, and update any callers to handle the sentinel error value appropriately; ensure any documentation/comments reflect the new error contract.firmware/sensor_microcontroller/src/FanFunctions.cpp-14-14 (1)
14-14:⚠️ Potential issue | 🟠 Major
pulse_counttype mismatch:int16_tis too narrow and inconsistent withpulse_count_single.
pulse_count_single(line 12) isuint32_t, butpulse_count[4]isint16_t. Anint16_toverflows at 32,767 pulses — at moderate fan speeds this wraps in minutes. When the delta calculation on line 39 spans a wrap boundary, the result cast touint32_tproduces a huge spurious RPM value.Use
uint32_tto matchpulse_count_singleand eliminate the overflow risk.🐛 Proposed fix
-static volatile int16_t pulse_count[4] = {0, 0, 0, 0}; -static volatile int16_t rpm[4] = {0, 0, 0, 0}; +static volatile uint32_t pulse_count[4] = {0, 0, 0, 0}; +static int16_t rpm[4] = {0, 0, 0, 0};Also update
last_pulse_count_teston line 30:-static int16_t last_pulse_count_test[4] = {0, 0, 0, 0}; +static uint32_t last_pulse_count_test[4] = {0, 0, 0, 0};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/FanFunctions.cpp` at line 14, The array pulse_count is declared as static volatile int16_t which can overflow and is inconsistent with pulse_count_single (uint32_t); change pulse_count to static volatile uint32_t pulse_count[4] to match pulse_count_single and prevent 16-bit wraparound, and also update the type of last_pulse_count_test to a uint32_t-compatible type (e.g., uint32_t) so comparisons and delta calculations between pulse_count, pulse_count_single, and last_pulse_count_test use the same unsigned 32-bit type.firmware/sensor_microcontroller/src/microRosFunctions.cpp-257-270 (1)
257-270:⚠️ Potential issue | 🟠 MajorInitialize
errorPublisherinmicroRosSetupbefore callingpublishError.Line 347 always publishes to
errorPublisher, butmicroRosSetup()never initializes that publisher in the ROS branch. This is a runtime validity issue for the setup path.🛠️ Suggested fix
`#ifdef` ROS // create publisher RCCHECK(rclc_publisher_init_best_effort( &sensorPublisher, &node, ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Sensors), sensorTopicName)); // Create fingerprint publisher RCCHECK(rclc_publisher_init_default( &fingerprintPublisher, &node, ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Fingerprint), fingerprintTopicName)); + // Create error publisher + RCCHECK(rclc_publisher_init_default( + &errorPublisher, + &node, + ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, SensorError), + "sensor_error"));Also applies to: 341-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp` around lines 257 - 270, The ROS branch in microRosSetup never initializes errorPublisher but publishError (called later) assumes it is valid; update microRosSetup to call rclc_publisher_init_default (or appropriate init, matching other publishers) to initialize errorPublisher using ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Error) and the errorTopicName before any publishError calls; ensure you use the same errorPublisher variable name and wrap the init with RCCHECK to mirror sensorPublisher/fingerprintPublisher initialization.
🟡 Minor comments (17)
firmware/sensor_microcontroller/lib/README-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorFix wording for readability and grammar.
A few phrases are awkward (
"an own separate directory","a contents of","find automatically dependent libraries scanning..."). Please tighten these sentences to improve clarity for new contributors.Also applies to: 30-31, 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/lib/README` around lines 5 - 6, Rewrite the awkward sentences in the README: replace "an own separate directory" with "its own directory", change "a contents of" to "the contents of", and rephrase "find automatically dependent libraries scanning..." to "automatically find dependent libraries by scanning..."; apply these same wording fixes to the other occurrences noted (lines referencing the same phrases).firmware/sensor_microcontroller/include/README-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorFix mismatched inline-code quoting.
#include'and.h'use mixed delimiters; render them with matching markdown backticks for consistency and readability.✏️ Proposed doc fix
-by including it, with the C preprocessing directive `#include'. +by including it, with the C preprocessing directive `#include`. -In C, the usual convention is to give header files names that end with `.h'. +In C, the usual convention is to give header files names that end with `.h`.Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/README` at line 7, Fix the mismatched inline-code quoting by replacing the mixed delimiters around preprocessor tokens with consistent backticks: render `#include` and file extensions like `.h` using matching Markdown code ticks (e.g., `#include`, `.h`) in the README text (affects the current line and the similar occurrence at line 28); update any other instances in the file that use single quotes around these tokens to use backticks instead.firmware/sensor_microcontroller/.gitignore-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorMissing
#— "PlatformIO files" is treated as a literal ignore pattern, not a comment.Without the
#prefix, Git interpretsPlatformIO filesas a glob pattern to match against file/directory names rather than a section header comment. This is almost certainly unintentional.🐛 Proposed fix
-PlatformIO files +# PlatformIO files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/.gitignore` at line 36, The line "PlatformIO files" in firmware/sensor_microcontroller/.gitignore is currently treated as a literal ignore pattern; change it to a comment by prefixing it with "#" (e.g., "# PlatformIO files") so it serves as a section header and not a glob pattern, ensuring patterns below remain effective and readable.firmware/sensor_microcontroller/README.md-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorHeading hierarchy skips a level (h1 → h3).
### Development Environent Setupis an h3 directly under the h1# Contributing Guidelines, violating Markdown heading-increment rules (MD001). It should be h2.📝 Proposed fix
-### Development Environent Setup +## Development Environment Setup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/README.md` at line 8, Change the heading "### Development Environent Setup" from an h3 to an h2 so it becomes "## Development Environent Setup" to fix the Markdown heading hierarchy violation (MD001); locate the string "Development Environent Setup" in README.md and update the leading hashes accordingly.firmware/sensor_microcontroller/README.md-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorStale product reference: "wheelchair" should reflect the tour guide project.
This README was carried over from the old wheelchair repo. Line 4 should describe the tour guide system.
📝 Proposed fix
-this repo is for all the code related to the microcontroller system for our wheelchair. +This directory contains all code related to the sensor microcontroller system for the tour guide robot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/README.md` at line 4, Update the README sentence that currently reads "this repo is for all the code related to the microcontroller system for our wheelchair." to reference the tour guide project instead; change the wording to something like "this repo contains all microcontroller firmware and related code for the tour guide system" so the description accurately reflects the project's scope in README.md.firmware/sensor_microcontroller/README.md-8-24 (1)
8-24:⚠️ Potential issue | 🟡 MinorMultiple spelling typos across the document.
The following typos were found:
Line Current Correction 8 EnvironentEnvironment14 RapspberryRaspberry21 corrispondingcorresponding24 decriptiondescription📝 Proposed fixes
-### Development Environent Setup +## Development Environment Setup-6. Upload the project to the Rapspberry Pi Pico using the PlatformIO upload button. +6. Upload the project to the Raspberry Pi Pico using the PlatformIO upload button.-Please put all `.c/cpp` files in the src directory and the corrisponding `.h` files in the include directory. +Please put all `.c/cpp` files in the src directory and the corresponding `.h` files in the include directory.-Please write a brief decription of what your code changed, and why you made those changes. +Please write a brief description of what your code changed, and why you made those changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/README.md` around lines 8 - 24, Fix multiple spelling typos in the README: change the section heading "Development Environent Setup" to "Development Environment Setup", correct "Rapspberry Pi Pico" to "Raspberry Pi Pico", update "corrisponding `.h` files" to "corresponding `.h` files", and change "decription" in the Pull Request Guidelines to "description"; ensure these exact replacements are applied in the README.md content so the heading, the Pico reference, the include-directory guidance, and the PR guidelines read correctly.firmware/motor_microcontroller/README.md-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorStale CI badge points to a different org and workflow
The badge references
WheelchairSeniorDesign/MotorMicrocontrollerwith workflowPlatformio_CI.yml, but this repo isTourGuideSeniorDesign/tour-guide-code-modulewith workflowmotor_microcontroller_CI.yml. The badge's URL is likely still referencing a workflow name or path that doesn't exist. Both the image source and the link target need updating.🔧 Proposed fix
-[](https://github.com/WheelchairSeniorDesign/MotorMicrocontroller/actions/workflows/Platformio_CI.yml) +[](https://github.com/TourGuideSeniorDesign/tour-guide-code-module/actions/workflows/motor_microcontroller_CI.yml)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/motor_microcontroller/README.md` at line 1, Update the stale CI badge in the README: change the image source and link target that currently point to WheelchairSeniorDesign/MotorMicrocontroller and Platformio_CI.yml so they reference the correct repository TourGuideSeniorDesign/tour-guide-code-module and the actual workflow filename motor_microcontroller_CI.yml; ensure both the badge URL and the href use the same repo/workflow names so the badge image and link resolve to the existing GitHub Actions workflow.firmware/sensor_microcontroller/src/ADCFunctions.cpp-21-24 (1)
21-24:⚠️ Potential issue | 🟡 MinorRetry limit is off-by-one.
At Line 21,
adc_count > 10allows an extra failed attempt before returning. If the intended max is 10 retries, switch to>= 10(or increment before the check).Suggested fix
- if (adc_count > 10) { + if (adc_count >= 10) { return true; } adc_count++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/ADCFunctions.cpp` around lines 21 - 24, The retry check for adc_count is off-by-one: currently it allows 11 attempts because it checks if (adc_count > 10) after incrementing; change the logic to enforce a maximum of 10 retries by either using if (adc_count >= 10) in place of if (adc_count > 10) or by incrementing adc_count before the check (move adc_count++ above the if). Update the condition that references adc_count in ADCFunctions.cpp so the loop/early return triggers exactly when the 10th retry is reached.firmware/sensor_microcontroller/include/FanFunctions.h-18-23 (1)
18-23:⚠️ Potential issue | 🟡 Minor
FanDutyCyclesusesint8_tbutsetFanIndividualacceptsuint8_tfor duty cycle.
int8_thas a max of 127, whileuint8_tgoes to 255. If duty cycles represent 0–255 (common for 8-bit PWM), values above 127 will be misinterpreted as negative when stored inFanDutyCycles. At minimum, aligning the types across the struct and the function signature would prevent subtle truncation bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/FanFunctions.h` around lines 18 - 23, The FanDutyCycles struct uses signed int8_t fields while the function setFanIndividual accepts uint8_t duty values, risking negative values for duty >127; update FanDutyCycles' members (fan_0_duty_cycle, fan_1_duty_cycle, fan_2_duty_cycle, fan_3_duty_cycle) to uint8_t to match setFanIndividual (or alternatively change setFanIndividual to int8_t if the API is intended to be signed), ensuring the types are consistent to prevent truncation/interpretation bugs.firmware/sensor_microcontroller/include/JoystickFunctions.h-18-24 (1)
18-24:⚠️ Potential issue | 🟡 MinorTruncated doc comment.
Line 19:
"Struct representing the reference displace"is missing the trailingment./** - * Struct representing the reference displace + * Struct representing the reference displacement. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/JoystickFunctions.h` around lines 18 - 24, Fix the truncated doc comment for the struct by updating the comment text to read "Struct representing the reference displacement" so the documentation above the RefDisplacement struct correctly spells "displacement"; locate the comment immediately preceding struct RefDisplacement and replace the existing truncated line with the complete phrase.src/autogiro_interfaces/msg/SensorError.msg-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorConsider adding a
lidar_errorfield.The PR includes
Lidar.msgand firmware references alidar_subscription_callback, so the lidar is an active tracked sensor. Omitting it fromSensorError.msgmeans lidar faults have no standard error-reporting channel alongside the other sensors.✏️ Suggested addition
bool joystick_adc_error bool ultrasonic_adc_error bool fingerprint_error bool imu_error +bool lidar_error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/autogiro_interfaces/msg/SensorError.msg` around lines 1 - 4, SensorError.msg is missing a lidar_error boolean even though the codebase includes Lidar.msg and a lidar_subscription_callback that monitors the lidar; add a new field named lidar_error to SensorError.msg (consistent with the existing bool fields joystick_adc_error, ultrasonic_adc_error, fingerprint_error, imu_error) so lidar faults can be reported via the same sensor error message type and update any publishers/subscribers (e.g., lidar_subscription_callback) to set this field when lidar faults are detected.firmware/sensor_microcontroller/src/PIRFunctions.cpp-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorMisleading
Serial.printlninsetupPIR()— prints "PIR detected" during initialization.This string implies a motion detection event, but it fires once at boot during pin configuration. Rename it to avoid confusion during debugging.
✏️ Proposed fix
- Serial.println("PIR detected"); + Serial.println("PIR sensors initialized");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/PIRFunctions.cpp` at line 16, In setupPIR() update the misleading Serial.println("PIR detected") that runs during initialization to a clear initialization message (e.g., "PIR initialized" or "PIR setup complete") so it doesn't appear as a motion event; locate the Serial.println call inside setupPIR() in PIRFunctions.cpp and replace the string to reflect initialization instead of detection.firmware/sensor_microcontroller/src/UltrasonicFunctions.cpp-11-11 (1)
11-11:⚠️ Potential issue | 🟡 MinorTypo: "dac" should be "adc".
The comment says "16 bit dac" but this value represents the ADC max, not a DAC.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/UltrasonicFunctions.cpp` at line 11, The comment on the ADC_SOLUTION macro has a typo referencing "dac" instead of "adc"; update the comment next to `#define` ADC_SOLUTION 26230 in UltrasonicFunctions.cpp to say it represents the maximum ADC value for a 16-bit ADC at 3.3V (e.g., change "Max value for a 16 bit dac at 3.3V" to "Max value for a 16-bit ADC at 3.3V" or similar).firmware/sensor_microcontroller/src/main.cpp-138-138 (1)
138-138:⚠️ Potential issue | 🟡 MinorHardcoded sentinel
fingerID = 2collides with valid fingerprint IDs.If a fingerprint with database ID 2 exists, it becomes indistinguishable from the "no scan performed" sentinel. Consider using
FINGERPRINT_NOFINGERor0xFF(255) as a sentinel instead, since valid enrolled IDs typically start at low values and 255 is unlikely to be used.🛡️ Proposed fix
- uint8_t fingerID = 2; + uint8_t fingerID = 0xFF; // sentinel: no scan performed ... - if(fingerID != 2){ + if(fingerID != 0xFF){ publishFingerprint(fingerID); }Also applies to: 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/main.cpp` at line 138, The code uses a hardcoded sentinel uint8_t fingerID = 2 which can collide with real fingerprint IDs; change the sentinel to an unmistakable value (e.g., use the library constant FINGERPRINT_NOFINGER or 0xFF) wherever fingerID is initialized or set (including the declaration in main.cpp and the other occurrences around the previous 161-163 region) and ensure all logic that checks for "no scan performed" compares against that new sentinel (and that fingerID remains uint8_t).firmware/motor_microcontroller/src/main.cpp-184-193 (1)
184-193:⚠️ Potential issue | 🟡 MinorComments contradict the code for
enableandbrakesignals.Line 184: comment says "enable the motor controller" but sets
enable = false. Line 192: comment says "disable brake" but setsbrake = true. If the signals are active-low, please update the comments to reflect that. Misleading comments are worse than no comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/motor_microcontroller/src/main.cpp` around lines 184 - 193, The comments around the enable/brake logic are incorrect relative to the assignments in main.cpp: the code sets enable = false and sets brake = true when speeds != 0 while comments state the opposite; either update the comments to indicate these signals are active-low (e.g., "enable is active-low: false -> enabled" and "brake is active-low: true -> braking") or change the assignments to match the current English comments. Locate the block using the symbols enable, brake and the check on refSpeedSensors.rightSpeed / refSpeedSensors.leftSpeed and either flip the boolean assignments to match the intent in the comments or change the comment text to explicitly state the active-low semantics so the code and comments are consistent.firmware/sensor_microcontroller/src/JoystickFunctions.cpp-129-130 (1)
129-130:⚠️ Potential issue | 🟡 MinorRedundant check:
abs()result is always ≥ 0.
diffis assigned fromabs(...), so it's always non-negative. The conditiondiff > -diffParam(i.e.,diff > -30) is always true. The effective check is justdiff < diffParam.If the intent was to check whether left and right speeds are "close enough," consider dropping the
abs()and checking the signed difference:Option A: simplify with abs
- int diff = abs(speeds.leftSpeed - speeds.rightSpeed); - if(diff > -diffParam && diff < diffParam){ + int diff = abs(speeds.leftSpeed - speeds.rightSpeed); + if(diff < diffParam){🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/JoystickFunctions.cpp` around lines 129 - 130, The current check uses int diff = abs(speeds.leftSpeed - speeds.rightSpeed) then tests diff > -diffParam && diff < diffParam, but abs(...) is non-negative so the first comparison is redundant; either remove the unnecessary diff > -diffParam and keep if (diff < diffParam) to use the absolute-distance semantics, or if you intended a signed-range check compute int diff = speeds.leftSpeed - speeds.rightSpeed and replace the condition with if (diff > -diffParam && diff < diffParam). Update the usage of diff and the conditional accordingly (symbols: diff, diffParam, speeds.leftSpeed, speeds.rightSpeed).firmware/sensor_microcontroller/src/FingerprintFunctions.cpp-88-91 (1)
88-91:⚠️ Potential issue | 🟡 Minor
while (!Serial)can block indefinitely on headless deployments.On boards without a USB-Serial monitor attached, this loop may never terminate. Consider adding a timeout to prevent hanging in production.
⏱️ Proposed fix with timeout
bool setupFingerprint() { - while (!Serial); // For Yun/Leo/Micro/Zero/... + { + unsigned long start = millis(); + while (!Serial && millis() - start < 3000); // timeout after 3s + } delay(100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/FingerprintFunctions.cpp` around lines 88 - 91, The setupFingerprint function uses "while (!Serial)" which can block indefinitely on headless devices; change this to a non-blocking wait with a timeout using millis(): record a start time, loop while !Serial but break if elapsed time exceeds a configurable timeout (e.g., a few seconds), and proceed whether or not Serial became available; update the Serial wait logic in setupFingerprint and any related initialization so the device never hangs if no USB-Serial is attached.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
.github/workflows/frontend_CI.yml.github/workflows/motor_microcontroller_CI.yml.github/workflows/sensor_microcontroller_CI.ymlfirmware/motor_microcontroller/.gitignorefirmware/motor_microcontroller/README.mdfirmware/motor_microcontroller/extra_packages/extra_packages.reposfirmware/motor_microcontroller/include/BatteryFunctions.hfirmware/motor_microcontroller/include/READMEfirmware/motor_microcontroller/include/RefSpeed.hfirmware/motor_microcontroller/include/globals.hfirmware/motor_microcontroller/include/microRosFunctions.hfirmware/motor_microcontroller/lib/READMEfirmware/motor_microcontroller/platformio.inifirmware/motor_microcontroller/src/BatteryFuctions.cppfirmware/motor_microcontroller/src/main.cppfirmware/motor_microcontroller/src/microRosFunctions.cppfirmware/motor_microcontroller/test/READMEfirmware/sensor_microcontroller/.gitignorefirmware/sensor_microcontroller/README.mdfirmware/sensor_microcontroller/extra_packages/extra_packages.reposfirmware/sensor_microcontroller/include/ADCFunctions.hfirmware/sensor_microcontroller/include/FanFunctions.hfirmware/sensor_microcontroller/include/FingerprintFunctions.hfirmware/sensor_microcontroller/include/IMUFunctions.hfirmware/sensor_microcontroller/include/JoystickFunctions.hfirmware/sensor_microcontroller/include/LidarFunctions.hfirmware/sensor_microcontroller/include/LightFunctions.hfirmware/sensor_microcontroller/include/PIRFunctions.hfirmware/sensor_microcontroller/include/PWMFunctions.hfirmware/sensor_microcontroller/include/READMEfirmware/sensor_microcontroller/include/UltrasonicFunctions.hfirmware/sensor_microcontroller/include/microRosFunctions.hfirmware/sensor_microcontroller/lib/READMEfirmware/sensor_microcontroller/platformio.inifirmware/sensor_microcontroller/src/ADCFunctions.cppfirmware/sensor_microcontroller/src/FanFunctions.cppfirmware/sensor_microcontroller/src/FingerprintFunctions.cppfirmware/sensor_microcontroller/src/IMUFunctions.cppfirmware/sensor_microcontroller/src/JoystickFunctions.cppfirmware/sensor_microcontroller/src/LidarFunctions.cppfirmware/sensor_microcontroller/src/LightFunctions.cppfirmware/sensor_microcontroller/src/PIRFunctions.cppfirmware/sensor_microcontroller/src/PWMFunctions.cppfirmware/sensor_microcontroller/src/UltrasonicFunctions.cppfirmware/sensor_microcontroller/src/main.cppfirmware/sensor_microcontroller/src/microRosFunctions.cppfirmware/sensor_microcontroller/test/READMEsrc/autogiro_interfaces/CMakeLists.txtsrc/autogiro_interfaces/msg/Battery.msgsrc/autogiro_interfaces/msg/Brake.msgsrc/autogiro_interfaces/msg/DacValues.msgsrc/autogiro_interfaces/msg/Fingerprint.msgsrc/autogiro_interfaces/msg/Lidar.msgsrc/autogiro_interfaces/msg/Light.msgsrc/autogiro_interfaces/msg/Motors.msgsrc/autogiro_interfaces/msg/RefSpeed.msgsrc/autogiro_interfaces/msg/SensorError.msg
| int16_t refSpeedR; // value sent to the motor controller for speed of right motor | ||
| int16_t refSpeedL; // value sent to the motor controller for speed of left motor | ||
| int16_t batteryValue; | ||
| refSpeed refSpeedSensors; |
There was a problem hiding this comment.
refSpeedSensors is uninitialized when ROS/ROS_DEBUG are not defined — garbage motor commands.
refSpeedSensors is only populated inside the #if defined(ROS) || defined(ROS_DEBUG) block (line 145), but it's used unconditionally at lines 185–218 to derive motor direction and speed. When neither macro is defined, the struct contains indeterminate values, which will drive the motors unpredictably.
🐛 Proposed fix — zero-initialize
-refSpeed refSpeedSensors;
+refSpeed refSpeedSensors{};Or, alternatively, ensure the non-ROS code path also populates refSpeedSensors (e.g., from a serial or direct sensor input).
Also applies to: 184-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/motor_microcontroller/src/main.cpp` at line 47, The refSpeedSensors
instance is left uninitialized when ROS/ROS_DEBUG are not defined, causing
garbage motor commands; fix by zero-initializing refSpeedSensors at declaration
(e.g., refSpeedSensors refSpeedSensors = {}; or memset) or ensure the non-ROS
code path populates it before use; update the declaration and/or add a fallback
initialization before any code that derives motor direction/speed from
refSpeedSensors so the struct has deterministic values.
| * @param adc - An instance of the Adafruit_ADS1115 class for the ADC | ||
| * @param i2c_addr - The I2C address of the ADC in hex | ||
| */ | ||
| bool adcInit(Adafruit_ADS1115 &adc, uint8_t i2c_addr); |
There was a problem hiding this comment.
Inverted adcInit return value — true signals failure, false signals success
In firmware/sensor_microcontroller/src/ADCFunctions.cpp (line 21), adcInit returns true when the retry limit is exceeded (i.e., initialization failed) and false on success. This is the opposite of the conventional C++ true = success pattern. Any caller that writes if (adcInit(...)) { /* proceed */ } will proceed with an uninitialised ADC.
Fix the implementation to align with convention, and update the Doxygen to document the semantics:
🐛 Proposed fix in ADCFunctions.cpp
if (adc_count > 10) {
- return true; // was: true = failure
+ return false; // false = initialization failed
}
...
}
- return false; // was: false = success
+ return true; // true = initialization succeededAlso update the @return Doxygen in ADCFunctions.h:
+ * `@return` true if ADC initialized successfully, false on failure
bool adcInit(Adafruit_ADS1115 &adc, uint8_t i2c_addr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/include/ADCFunctions.h` at line 16, The
adcInit function currently returns true on failure and false on success; change
its implementation in ADCFunctions.cpp (function adcInit(Adafruit_ADS1115 &adc,
uint8_t i2c_addr)) so it returns true when initialization succeeds and false
when it fails (e.g., after exhausting retries), and update the Doxygen `@return`
in ADCFunctions.h to state "@return true on successful initialization, false on
failure"; ensure all return points in adcInit are inverted accordingly and that
no other logic in adcInit relies on the old inverted convention.
|
|
||
| //microRosSetup(1, nodeName, sensorTopicName, fingerprintTopicName); | ||
|
|
||
| #elif ROS_DEBUG |
There was a problem hiding this comment.
#elif ROS_DEBUG / #elif DEBUG should use defined().
#elif ROS_DEBUG checks the macro's integer value, not its existence. If the build system defines -DROS_DEBUG (with no value), the macro expands to nothing and the preprocessor evaluates #elif with an empty expression (treated as 0 → false). The #ifdef ROS on line 50 and #if defined(ROS) on line 15 use the correct form. This should be consistent throughout.
🐛 Proposed fix
- `#elif` ROS_DEBUG
+ `#elif` defined(ROS_DEBUG)Apply the same fix at line 164 and line 170:
- `#elif` ROS_DEBUG
+ `#elif` defined(ROS_DEBUG)
...
- `#elif` DEBUG
+ `#elif` defined(DEBUG)Also applies to: 164-164, 170-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/main.cpp` at line 61, The preprocessor
checks use bare "#elif ROS_DEBUG" and "#elif DEBUG" which test numeric value
rather than macro existence; change these to use the defined() form (e.g.
replace "#elif ROS_DEBUG" with "#elif defined(ROS_DEBUG)" and "#elif DEBUG" with
"#elif defined(DEBUG)") so presence-only defines work correctly; update every
occurrence (the ones paired with the existing "#ifdef ROS" and "#if
defined(ROS)" checks and the additional spots noted) to use defined(MACRO) for
consistency.
| watchdog_enable(5000, 1); // updating the watchdog// set the watchdog to run at 5s interval | ||
|
|
||
| ultrasonic_adc_error = adcInit(ultrasonicAdc, 0x49); //default address | ||
| joystick_adc_error = adcInit(joystickAdc, 0x48); //default address | ||
| imu_error = imuInit(icm, ICM20948_ACCEL_RANGE_2_G, ICM20948_GYRO_RANGE_250_DPS, AK09916_MAG_DATARATE_10_HZ); | ||
| fingerprint_error = setupFingerprint(); | ||
| Serial.print("Fingerprint error: "); | ||
| setAllFans(startDutyCycles); | ||
| setupRPMCounter(); | ||
| setupLight(); |
There was a problem hiding this comment.
Watchdog enabled before sensor init — risk of reset loop.
The watchdog is armed with a 5-second timeout at line 71, but sensor initialization follows (lines 73–80). The fingerprint sensor retry loop alone can take >5 seconds (up to 11 retries). If the total init time exceeds 5 seconds, the watchdog fires and the board enters a reset loop.
Move the watchdog_enable call to after all initialization is complete, or add watchdog_update() calls within the long init sequences.
🐛 Proposed fix — move watchdog enable after init
- watchdog_enable(5000, 1); // updating the watchdog// set the watchdog to run at 5s interval
-
ultrasonic_adc_error = adcInit(ultrasonicAdc, 0x49);
joystick_adc_error = adcInit(joystickAdc, 0x48);
imu_error = imuInit(icm, ICM20948_ACCEL_RANGE_2_G, ICM20948_GYRO_RANGE_250_DPS, AK09916_MAG_DATARATE_10_HZ);
fingerprint_error = setupFingerprint();
...
setupRPMCounter();
setupLight();
+
+ watchdog_enable(5000, 1); // set the watchdog to run at 5s interval🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/main.cpp` around lines 71 - 80, The
watchdog is enabled with watchdog_enable(5000, 1) before long sensor init calls
(adcInit, imuInit, setupFingerprint) which can exceed 5s and cause reset loops;
move the watchdog_enable call to after all initialization (after
setupLight()/setupRPMCounter()/setAllFans) or, if you must keep it earlier,
insert periodic watchdog_update() calls inside long routines (notably inside
setupFingerprint's retry loop) so the watchdog is refreshed while waiting;
update the code to call watchdog_enable only once initialization is complete or
ensure watchdog_update is called during any multi-second retry loops.
| #ifdef ROS | ||
| if (!joystick_adc_error || !ultrasonic_adc_error || !fingerprint_error || !imu_error) { | ||
| publishError(joystick_adc_error, ultrasonic_adc_error, fingerprint_error, imu_error); | ||
| error_timer = 500; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Inverted error-publish condition — publishes when sensors are healthy, not when they fail.
!joystick_adc_error || !ultrasonic_adc_error || !fingerprint_error || !imu_error is true when any sensor initialized successfully. This means errors are published even when all sensors are fine, and the reduced error_timer (500ms) is set unnecessarily. The intent appears to be publishing when any sensor failed.
🐛 Proposed fix
- if (!joystick_adc_error || !ultrasonic_adc_error || !fingerprint_error || !imu_error) {
+ if (joystick_adc_error || ultrasonic_adc_error || fingerprint_error || imu_error) {
publishError(joystick_adc_error, ultrasonic_adc_error, fingerprint_error, imu_error);
error_timer = 500;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifdef ROS | |
| if (!joystick_adc_error || !ultrasonic_adc_error || !fingerprint_error || !imu_error) { | |
| publishError(joystick_adc_error, ultrasonic_adc_error, fingerprint_error, imu_error); | |
| error_timer = 500; | |
| } | |
| #endif | |
| `#ifdef` ROS | |
| if (joystick_adc_error || ultrasonic_adc_error || fingerprint_error || imu_error) { | |
| publishError(joystick_adc_error, ultrasonic_adc_error, fingerprint_error, imu_error); | |
| error_timer = 500; | |
| } | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/main.cpp` around lines 82 - 87, The
condition currently publishes errors when sensors are healthy because it checks
!joystick_adc_error || !ultrasonic_adc_error || !fingerprint_error ||
!imu_error; change it to publish only when any sensor failed by using the plain
error flags: joystick_adc_error || ultrasonic_adc_error || fingerprint_error ||
imu_error, then call publishError(joystick_adc_error, ultrasonic_adc_error,
fingerprint_error, imu_error) and set error_timer = 500 inside that branch; keep
this logic inside the existing `#ifdef` ROS block so the behavior only runs under
ROS.
| #elif ROS_DEBUG | ||
| // create publisher | ||
| RCCHECK(rclc_publisher_init_best_effort( | ||
| &sensorPublisher, | ||
| &node, | ||
| ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, RefSpeed), | ||
| topicName)); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP 'bool create_entities\s*\(|\btopicName\b' firmware/sensor_microcontroller/src/microRosFunctions.cpp -C3Repository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 981
🏁 Script executed:
cat -n firmware/sensor_microcontroller/src/microRosFunctions.cpp | sed -n '85,150p'Repository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 2363
🏁 Script executed:
rg -n 'create_entities\s*\(' firmware/sensor_microcontroller/src/microRosFunctions.cppRepository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 201
🏁 Script executed:
cat -n firmware/sensor_microcontroller/src/microRosFunctions.cpp | sed -n '200,250p'Repository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 2217
🏁 Script executed:
cat -n firmware/sensor_microcontroller/src/microRosFunctions.cpp | sed -n '1,90p'Repository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 2945
🏁 Script executed:
rg -n 'topicName\b' firmware/sensor_microcontroller/src/microRosFunctions.cppRepository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 241
🏁 Script executed:
cat -n firmware/sensor_microcontroller/src/microRosFunctions.cpp | sed -n '230,300p'Repository: TourGuideSeniorDesign/tour-guide-code-module
Length of output: 2780
Fix undefined topicName in ROS_DEBUG create_entities().
Line 135 references topicName, but create_entities() has no parameters and is called from microRosTick() with no arguments, leaving topicName undefined in scope. ROS_DEBUG builds will fail at compile time. Use a hardcoded topic name instead, matching the ROS build pattern.
🛠️ Suggested fix
`#elif` ROS_DEBUG
// create publisher
RCCHECK(rclc_publisher_init_best_effort(
&sensorPublisher,
&node,
ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, RefSpeed),
- topicName));
+ "ref_speed"));
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp` around lines 129 -
136, create_entities() references an undefined variable topicName when
initializing sensorPublisher under the ROS_DEBUG branch, causing compile
failures because create_entities() is called from microRosTick() with no
parameters; fix by replacing the undefined topicName with a hardcoded topic
string (matching the ROS build pattern) inside create_entities() where
RCCHECK(rclc_publisher_init_best_effort(&sensorPublisher, &node,
ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, RefSpeed), topicName)) is
invoked, ensuring the publisher is created with a concrete topic name and no
function signature changes to create_entities() or microRosTick().
| RCCHECK(rclc_executor_add_timer(&executor, &timer)); | ||
|
|
There was a problem hiding this comment.
Guard ROS-only entities during executor registration and teardown.
Line 171/Line 321 add timer in all builds, while timer creation is ROS-only. Line 195/Line 196 finalize ROS-only entities even in ROS_DEBUG. This can force RCCHECK into error_loop() on disconnect. Also, errorPublisher is created (Line 115-Line 119) but never finalized.
🛠️ Suggested fix
- RCCHECK(rclc_executor_add_timer(&executor, &timer));
+ `#ifdef` ROS
+ RCCHECK(rclc_executor_add_timer(&executor, &timer));
+ `#endif`
...
- RCCHECK(rcl_publisher_fini(&fingerprintPublisher, &node));
- RCCHECK(rcl_timer_fini(&timer));
+ `#ifdef` ROS
+ RCCHECK(rcl_publisher_fini(&fingerprintPublisher, &node));
+ RCCHECK(rcl_publisher_fini(&errorPublisher, &node));
+ RCCHECK(rcl_timer_fini(&timer));
+ `#endif`
...
- RCCHECK(rclc_executor_add_timer(&executor, &timer));
+ `#ifdef` ROS
+ RCCHECK(rclc_executor_add_timer(&executor, &timer));
+ `#endif`Also applies to: 195-199, 321-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp` around lines 171 -
172, Guard registration and teardown of ROS-only entities so they only run when
the timer/publisher were actually created: wrap the call to
rclc_executor_add_timer(&executor, &timer) (and the corresponding
rcl_timer_fini, rcl_publisher_fini and rclc_executor_fini calls) in the same
ROS-only conditional used when creating the timer and errorPublisher, or track
an "initialized" flag for timer/errorPublisher and check it before finalizing;
ensure RCCHECK is not invoked on uninitialized objects (avoid calling RCCHECK on
rcl_* fns when the entities weren't created) and add a proper rcl_publisher_fini
for errorPublisher to finalize it when it was created to prevent RCCHECK from
hitting error_loop().
| float clock_freq = 125000000.0; // Default RP2040 system clock (125 MHz) | ||
| uint16_t wrap = clock_freq / frequency - 1; | ||
| pwm_set_wrap(slice, wrap); | ||
|
|
||
| uint16_t level = (wrap * duty_cycle) / 100.0; | ||
| pwm_set_chan_level(slice, channel, level); |
There was a problem hiding this comment.
Guard invalid frequency/duty_cycle before computing wrap and level.
Line 16 can divide by zero (frequency <= 0), and Lines 19-20 can program invalid levels when duty_cycle is outside [0, 100]. This can produce undefined PWM behavior.
🔧 Proposed fix
void setPWM(uint pin, float frequency, float duty_cycle) {
+ if (frequency <= 0.0f) {
+ return;
+ }
+ if (duty_cycle < 0.0f) duty_cycle = 0.0f;
+ if (duty_cycle > 100.0f) duty_cycle = 100.0f;
+
uint slice = pwm_gpio_to_slice_num(pin);
uint channel = pwm_gpio_to_channel(pin);
gpio_set_function(pin, GPIO_FUNC_PWM);
- float clock_freq = 125000000.0; // Default RP2040 system clock (125 MHz)
- uint16_t wrap = clock_freq / frequency - 1;
+ float clock_freq = 125000000.0f; // Default RP2040 system clock (125 MHz)
+ float wrap_f = (clock_freq / frequency) - 1.0f;
+ if (wrap_f < 0.0f) wrap_f = 0.0f;
+ if (wrap_f > 65535.0f) wrap_f = 65535.0f;
+ uint16_t wrap = static_cast<uint16_t>(wrap_f);
pwm_set_wrap(slice, wrap);
- uint16_t level = (wrap * duty_cycle) / 100.0;
+ uint16_t level = static_cast<uint16_t>((wrap * duty_cycle) / 100.0f);
pwm_set_chan_level(slice, channel, level);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/PWMFunctions.cpp` around lines 15 - 20,
Validate inputs before computing wrap/level: ensure frequency > 0 (return or
error out if not) and compute raw_wrap = clock_freq / frequency, then convert to
an integer wrap = max(0, (int32_t)raw_wrap - 1) before calling
pwm_set_wrap(slice, wrap) to avoid negative/underflow values; clamp duty_cycle
into [0, 100], compute level = (wrap * clamped_duty_cycle) / 100.0 and ensure
level is bounded to [0, wrap] before calling pwm_set_chan_level(slice, channel,
level); reference the variables/functions frequency, duty_cycle, wrap, level,
slice, channel, pwm_set_wrap and pwm_set_chan_level when making the changes.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
firmware/sensor_microcontroller/include/microRosFunctions.h (2)
52-57:⚠️ Potential issue | 🔴 Critical
transmitMsgsignature mismatch in ROS_DEBUG block.The declaration
void transmitMsg(RefSpeed omegaRef)has a single parameter, but the implementation and call sites use two parameters (RefDisplacement thetaRef, RefSpeed omegaRef). This will cause compilation errors when building withROS_DEBUGdefined.Update the declaration to match:
-void transmitMsg(RefSpeed omegaRef); +void transmitMsg(RefDisplacement thetaRef, RefSpeed omegaRef);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/microRosFunctions.h` around lines 52 - 57, The prototype for transmitMsg is wrong: change the declaration of transmitMsg to accept both parameters used by its implementation and callers—RefDisplacement and RefSpeed—so the signature matches (e.g., transmitMsg(RefDisplacement thetaRef, RefSpeed omegaRef)). Update the declaration in the header where transmitMsg is declared so it matches the implementation and all call sites (ensure RefDisplacement is in scope or forward-declared as needed).
37-42:⚠️ Potential issue | 🔴 CriticalCallback declarations missing for ROS_DEBUG builds.
The subscription callback declarations (
fan_subscription_callback,light_subscription_callback,lidar_subscription_callback) are guarded by#ifdef ROS, but the executor registration code inmicroRosFunctions.cpp(lines 161-170) references these callbacks unconditionally in both ROS and ROS_DEBUG builds. This will cause undefined reference errors when compiling withROS_DEBUG.Move these declarations outside the conditional block so they are visible in both build configurations.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/include/microRosFunctions.h` around lines 37 - 42, The three subscription callback declarations (fan_subscription_callback, light_subscription_callback, lidar_subscription_callback) must be made visible in all build configs: move or change their declarations so they are not inside an `#ifdef` ROS block in the header and are declared unconditionally with the same signature (const void *msgin), because the executor registration code unconditionally references these symbols; update the header to remove the `#ifdef` guard around those prototypes so the linker sees them in ROS_DEBUG builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@firmware/sensor_microcontroller/include/microRosFunctions.h`:
- Around line 52-57: The prototype for transmitMsg is wrong: change the
declaration of transmitMsg to accept both parameters used by its implementation
and callers—RefDisplacement and RefSpeed—so the signature matches (e.g.,
transmitMsg(RefDisplacement thetaRef, RefSpeed omegaRef)). Update the
declaration in the header where transmitMsg is declared so it matches the
implementation and all call sites (ensure RefDisplacement is in scope or
forward-declared as needed).
- Around line 37-42: The three subscription callback declarations
(fan_subscription_callback, light_subscription_callback,
lidar_subscription_callback) must be made visible in all build configs: move or
change their declarations so they are not inside an `#ifdef` ROS block in the
header and are declared unconditionally with the same signature (const void
*msgin), because the executor registration code unconditionally references these
symbols; update the header to remove the `#ifdef` guard around those prototypes so
the linker sees them in ROS_DEBUG builds.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
firmware/sensor_microcontroller/src/microRosFunctions.cpp (2)
138-144:⚠️ Potential issue | 🔴 CriticalFix undefined
topicNameincreate_entities()ROS_DEBUG path.Line 143 uses
topicName, butcreate_entities()has no such variable in scope. This breaks ROS_DEBUG compilation.🛠️ Suggested fix
`#elif` ROS_DEBUG // create publisher RCCHECK(rclc_publisher_init_best_effort( &sensorPublisher, &node, ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, RefSpeed), - topicName)); + "ref_speed")); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp` around lines 138 - 144, In create_entities() the ROS_DEBUG path calls rclc_publisher_init_best_effort with an undefined topicName; define or use the correct topic string variable in scope (e.g., add a local const char* topicName initialized to the same topic used elsewhere or replace topicName with the existing topic variable) so sensorPublisher is initialized with a valid topic; update the ROS_DEBUG block around rclc_publisher_init_best_effort (&sensorPublisher, &node, ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, RefSpeed), topicName) accordingly.
175-175:⚠️ Potential issue | 🔴 CriticalGuard timer/exclusive ROS entities consistently during add/fini.
Line 175 and Line 317 always add
timer, buttimeris only initialized in ROS builds. Also, Line 198/Line 199 teardown runs even when those entities were never created (ROS_DEBUG), anderrorPublisheris never finalized.This can force
RCCHECKintoerror_loop()on disconnect/reconnect.🛠️ Suggested fix
RCCHECK(rclc_executor_add_subscription(&executor, &lidarSubscriber, &lidarMsg, &lidar_subscription_callback, ON_NEW_DATA)); - RCCHECK(rclc_executor_add_timer(&executor, &timer)); + `#ifdef` ROS + RCCHECK(rclc_executor_add_timer(&executor, &timer)); + `#endif` ... RCCHECK(rcl_subscription_fini(&fanSubscriber, &node)); RCCHECK(rcl_subscription_fini(&lidarSubscriber, &node)); RCCHECK(rcl_subscription_fini(&lightSubscriber, &node)); RCCHECK(rcl_publisher_fini(&sensorPublisher, &node)); - RCCHECK(rcl_publisher_fini(&fingerprintPublisher, &node)); - RCCHECK(rcl_timer_fini(&timer)); + `#ifdef` ROS + RCCHECK(rcl_publisher_fini(&fingerprintPublisher, &node)); + RCCHECK(rcl_publisher_fini(&errorPublisher, &node)); + RCCHECK(rcl_timer_fini(&timer)); + `#endif` RCCHECK(rclc_executor_fini(&executor)); ... - RCCHECK(rclc_executor_add_timer(&executor, &timer)); + `#ifdef` ROS + RCCHECK(rclc_executor_add_timer(&executor, &timer)); + `#endif`Also applies to: 194-200, 317-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp` at line 175, The code unconditionally calls RCCHECK(rclc_executor_add_timer(&executor, &timer)) and always runs teardown paths even when ROS entities (timer, errorPublisher) were never created; add guards that track creation (e.g., bool timer_created and error_publisher_created) or wrap the add/fini calls with the same ROS build conditional used for initializing them so rclc_executor_add_timer and rcl_publisher_fini are only invoked if timer and errorPublisher were actually initialized; update the teardown block (the code around symbols timer, executor, errorPublisher, and their RCCHECK/fini calls) to check those flags (or the ROS `#ifdef`) before calling RCCHECK or rcl_*_fini to avoid hitting error_loop() on disconnect/reconnect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp`:
- Around line 264-275: microRosSetup() currently initializes sensorPublisher and
fingerprintPublisher but never initializes errorPublisher, while publishError()
always uses errorPublisher; update microRosSetup() to initialize errorPublisher
(use rclc_publisher_init_best_effort or rclc_publisher_init_default consistent
with the other publishers) with the node, the correct message type support
(ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Error) or the actual
Error message type used by publishError()), and errorTopicName so that
publishError() has a valid publisher to send error messages through.
---
Duplicate comments:
In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp`:
- Around line 138-144: In create_entities() the ROS_DEBUG path calls
rclc_publisher_init_best_effort with an undefined topicName; define or use the
correct topic string variable in scope (e.g., add a local const char* topicName
initialized to the same topic used elsewhere or replace topicName with the
existing topic variable) so sensorPublisher is initialized with a valid topic;
update the ROS_DEBUG block around rclc_publisher_init_best_effort
(&sensorPublisher, &node, ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg,
RefSpeed), topicName) accordingly.
- Line 175: The code unconditionally calls
RCCHECK(rclc_executor_add_timer(&executor, &timer)) and always runs teardown
paths even when ROS entities (timer, errorPublisher) were never created; add
guards that track creation (e.g., bool timer_created and
error_publisher_created) or wrap the add/fini calls with the same ROS build
conditional used for initializing them so rclc_executor_add_timer and
rcl_publisher_fini are only invoked if timer and errorPublisher were actually
initialized; update the teardown block (the code around symbols timer, executor,
errorPublisher, and their RCCHECK/fini calls) to check those flags (or the ROS
`#ifdef`) before calling RCCHECK or rcl_*_fini to avoid hitting error_loop() on
disconnect/reconnect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90a0c115-ce4f-4a65-b191-a9fdf69ad84e
📒 Files selected for processing (2)
firmware/sensor_microcontroller/src/microRosFunctions.cppfrontend/src/renderer/src/types/ros-msgs.gen.ts
| #ifdef ROS | ||
| // create publisher | ||
| RCCHECK(rclc_publisher_init_best_effort( | ||
| &sensorPublisher, &node, | ||
| ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Sensors), | ||
| sensorTopicName)); | ||
| // Create fingerprint publisher | ||
| RCCHECK(rclc_publisher_init_default( | ||
| &fingerprintPublisher, &node, | ||
| ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Fingerprint), | ||
| fingerprintTopicName)); | ||
|
|
There was a problem hiding this comment.
Initialize errorPublisher in ROS microRosSetup() to match publishError().
publishError() (Line 335-344) always publishes through errorPublisher, but ROS microRosSetup() (Line 264-275) never initializes that publisher. Result: error messages are dropped/fail silently.
🛠️ Suggested fix
`#ifdef` ROS
// create publisher
RCCHECK(rclc_publisher_init_best_effort(
&sensorPublisher, &node,
ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Sensors),
sensorTopicName));
// Create fingerprint publisher
RCCHECK(rclc_publisher_init_default(
&fingerprintPublisher, &node,
ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Fingerprint),
fingerprintTopicName));
+ // Create error publisher
+ RCCHECK(rclc_publisher_init_default(
+ &errorPublisher, &node,
+ ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, SensorError),
+ "sensor_error"));Also applies to: 335-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/sensor_microcontroller/src/microRosFunctions.cpp` around lines 264 -
275, microRosSetup() currently initializes sensorPublisher and
fingerprintPublisher but never initializes errorPublisher, while publishError()
always uses errorPublisher; update microRosSetup() to initialize errorPublisher
(use rclc_publisher_init_best_effort or rclc_publisher_init_default consistent
with the other publishers) with the node, the correct message type support
(ROSIDL_GET_MSG_TYPE_SUPPORT(autogiro_interfaces, msg, Error) or the actual
Error message type used by publishError()), and errorTopicName so that
publishError() has a valid publisher to send error messages through.
1 participant
Summary by CodeRabbit
New Features
Documentation