Conversation
*Moved to Science-Aux
TMC-Firmware wrapper
to pinouts
(finally, pain and Agony)
There was a problem hiding this comment.
Pull request overview
This PR performs a substantial cleanup and restructuring of the Science subsystem for SAR, introducing a new Science-Aux component and updating the protocol buffer definitions. The changes modernize the hardware interface, rename components to better reflect their actual functions (e.g., "scoop" → "auger", "subsurface" → "linear_slider"), and remove deprecated functionality.
Changes:
- Introduced new Science-Aux subsystem with separate hardware control (auger motors, servos, LIDAR, temperature/humidity sensor, current sensor)
- Updated protocol buffer schemas to version 0.4.9.1 with new field names and Version message support
- Removed deprecated components (Scooper class, temp_humidity sensor from main Science, multiple pump configurations, unused motors)
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Science/src/version.pb.h | Added new auto-generated protocol buffer header for version tracking |
| Science/src/version.pb.c | Added corresponding protocol buffer implementation |
| Science/src/utils | Removed submodule reference |
| Science/src/tmc | Removed submodule reference |
| Science/src/temp_humidity/temp_humidity.h | Removed deprecated temperature/humidity sensor header |
| Science/src/temp_humidity/temp_humidity.cpp | Removed deprecated temperature/humidity sensor implementation |
| Science/src/scooper/scooper.h | Removed deprecated Scooper class header |
| Science/src/scooper/scooper.cpp | Removed deprecated Scooper class implementation |
| Science/src/science.pb.h | Updated protocol buffer with renamed fields and new version support |
| Science/src/science.pb.c | Updated protocol buffer implementation to version 0.4.9.1 |
| Science/src/pumps/pumps.h | Updated pump pin definitions and added TODO comment |
| Science/src/pumps/pumps.cpp | Updated pump timing constants with descriptive comments and refined fill logic |
| Science/src/motors/motors.h | Removed deprecated motor references |
| Science/src/motors/motors.cpp | Simplified to single carousel motor with corrected method names |
| Science/src/co2/co2_sensor.h | Changed return type from float to int |
| Science/src/co2/co2_sensor.cpp | Added calibration offset and fixed increment operator |
| Science/src/carousel/carousel.h | Removed servo-related functionality |
| Science/src/carousel/carousel.cpp | Removed funnel servo logic, commented out unfinished features |
| Science/pinouts.h | Updated pinout declarations for new architecture |
| Science/motor_pinouts.h | Modernized motor configuration syntax |
| Science/Science.ino | Updated main loop to use new serial interface and removed deprecated components |
| Science-Aux/src/version.pb.h | Added version protocol buffer for auxiliary subsystem |
| Science-Aux/src/version.pb.c | Added version protocol buffer implementation |
| Science-Aux/src/utils | Added submodule reference |
| Science-Aux/src/tmc | Added submodule reference |
| Science-Aux/src/temp_humidity/temp_humidity.h | Added new temperature/humidity sensor interface |
| Science-Aux/src/temp_humidity/temp_humidity.cpp | Added new temperature/humidity sensor implementation |
| Science-Aux/src/science.pb.h | Added protocol buffer header for auxiliary subsystem |
| Science-Aux/src/science.pb.c | Added protocol buffer implementation |
| Science-Aux/src/lidar/src/LIDARLite.h | Added LIDAR sensor library header |
| Science-Aux/src/lidar/src/LIDARLite.cpp | Added LIDAR sensor library implementation |
| Science-Aux/src/lidar/lidar.h | Added LIDAR wrapper class header |
| Science-Aux/src/lidar/lidar.cpp | Added LIDAR wrapper implementation |
| Science-Aux/src/current_sensor/current_sensor.h | Added current sensor class with filtering |
| Science-Aux/src/current_sensor/current_sensor.cpp | Added current sensor implementation with adaptive filtering |
| Science-Aux/src/auger_servo/auger_servo.h | Added auger servo control class |
| Science-Aux/src/auger_servo/auger_servo.cpp | Added auger servo implementation |
| Science-Aux/pinouts.h | Added pinout definitions for auxiliary subsystem |
| Science-Aux/Science-Aux.ino | Added main program for auxiliary subsystem |
| Science-Aux/.vscode/settings.json | Added VS Code configuration |
| Protobuf | Updated submodule reference |
| .gitmodules | Added submodule entries for Science-Aux dependencies |
Comments suppressed due to low confidence (4)
Science/src/pumps/pumps.h:1
- Corrected spelling of 'seperate' to 'separate'.
Science/src/pumps/pumps.cpp:1 - Corrected spelling of 'H202' to 'H2O2' (hydrogen peroxide formula).
Science/src/pumps/pumps.cpp:1 - This arithmetic results in a negative delay value (-6700ms) since pumpDelay2 (3300) is less than pumpDelay3 (10000). The delay() function with a negative value will cause incorrect behavior. The operands should likely be swapped or the logic reconsidered.
Science/src/pumps/pumps.cpp:1 - This arithmetic results in a small delay (3400ms), but based on the sequence, this appears to be using the wrong delay constants. Review the intended pump timing sequence to ensure the correct delays are applied.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dirtLinear.presetup(); | ||
|
|
||
| scooperArm.setup(); | ||
| dirtCarousel.preSetup(); |
There was a problem hiding this comment.
Inconsistent method naming: 'preSetup' here vs 'presetup' in commented-out code on previous lines. The casing should be standardized across the codebase.
| dirtCarousel.preSetup(); | |
| dirtCarousel.presetup(); |
| // stepper.moveby(1); | ||
| // } | ||
| // stepper.block(); | ||
| // detachInterupt(digitalPinToInterrupt(indexPin)); |
There was a problem hiding this comment.
Corrected spelling of 'detachInterupt' to 'detachInterrupt' on line 23.
| // detachInterupt(digitalPinToInterrupt(indexPin)); | |
| // detachInterrupt(digitalPinToInterrupt(indexPin)); |
| // pinMode(indexPin,INPUT); | ||
| // attachInterrupt(digitalPinToInterrupt(indexPin), Home, CHANGE); | ||
| // while (!atHome){ | ||
| // stepper.moveby(1); |
There was a problem hiding this comment.
Inconsistent method naming: 'moveby' should likely be 'moveBy' to match the camelCase convention used elsewhere in the codebase (e.g., 'moveBySteps').
| // stepper.moveby(1); | |
| // stepper.moveBy(1); |
| }; | ||
| StepperMotor dirtCarouselMotor(StepperGeneralConfig{ | ||
| .name = "dirtCarousel", | ||
| .steps_per_unit = microsteps_per_deg * -1, |
There was a problem hiding this comment.
The variable name 'microsteps_per_deg' is inconsistent with 'microsteps_per_deg' used elsewhere. Verify this is the correct identifier or if it should match the naming convention from other files.
| .steps_per_unit = microsteps_per_deg * -1, | |
| .steps_per_unit = MICROSTEPS_PER_DEG * -1, |
| Version version = {major: 1, minor: 1}; | ||
|
|
There was a problem hiding this comment.
The version variable is declared but never used in this file. Consider removing it or integrating it into the version tracking system if it's intended to be sent with ScienceData.
| Version version = {major: 1, minor: 1}; |
| StepperMotor linearSlider( | ||
| StepperGeneralConfig{ | ||
| .name ="linearSlider", | ||
| .steps_per_unit = microsteps_per_deg, |
There was a problem hiding this comment.
Inconsistent naming: 'microsteps_per_deg' here doesn't match 'microsteps_per_deg' used in Science/motor_pinouts.h. The variable name should be consistent across both files.
Science subsystem clean up for SAR