Implemented direct joystick logs - #1
Conversation
WalkthroughAdds joystick displacement support. Introduces RefDisplacement and joystickToDisplacement(), reads ADC to compute longitudinal/lateral displacement. Updates transmitMsg signature to include displacement in both ROS and ROS_DEBUG builds. main.cpp now computes thetaRef from joystick ADC and passes it to transmitMsg alongside existing speed and sensor data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant J as Joystick ADC
participant F as JoystickFunctions
participant M as main.cpp (loop)
participant U as microRosFunctions
participant R as micro-ROS Transport
rect rgba(230,245,255,0.6)
note over M: Each loop iteration
M->>J: Read raw joystick channels
M->>F: joystickToDisplacement(adc)
F-->>M: RefDisplacement (longDisp, latDisp)
M->>U: transmitMsg(thetaRef, omegaRef, sensors...)
U->>R: Publish message (disp + speed + sensors)
end
sequenceDiagram
autonumber
participant J as Joystick ADC
participant F as JoystickFunctions
participant M as main.cpp (loop)
participant U as microRosFunctions (ROS_DEBUG)
rect rgba(240,255,240,0.6)
note over M: ROS_DEBUG build
M->>F: joystickToDisplacement(adc)
F-->>M: RefDisplacement
M->>U: transmitMsg(thetaRef, omegaRef)
U-->>M: Debug output includes displacement
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 3
🧹 Nitpick comments (2)
MicrocontrollerCode/src/main.cpp (2)
154-154: Optional: Add space after comma for consistency.Minor formatting suggestion for readability.
Apply this diff:
- transmitMsg(thetaRef,omegaRef, usDistances, pirSensors, fanSpeeds, imuData); + transmitMsg(thetaRef, omegaRef, usDistances, pirSensors, fanSpeeds, imuData);
167-167: Optional: Add space after comma for consistency.Minor formatting suggestion for readability.
Apply this diff:
- transmitMsg(thetaRef,omegaRef); + transmitMsg(thetaRef, omegaRef);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MicrocontrollerCode/include/JoystickFunctions.h(1 hunks)MicrocontrollerCode/src/JoystickFunctions.cpp(1 hunks)MicrocontrollerCode/src/main.cpp(3 hunks)MicrocontrollerCode/src/microRosFunctions.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MicrocontrollerCode/include/JoystickFunctions.h (1)
MicrocontrollerCode/src/JoystickFunctions.cpp (4)
joystickToSpeed(34-147)joystickToSpeed(34-34)joystickToDisplacement(12-32)joystickToDisplacement(12-12)
MicrocontrollerCode/src/main.cpp (2)
MicrocontrollerCode/src/JoystickFunctions.cpp (2)
joystickToDisplacement(12-32)joystickToDisplacement(12-12)MicrocontrollerCode/src/microRosFunctions.cpp (4)
transmitMsg(376-405)transmitMsg(376-376)transmitMsg(411-418)transmitMsg(411-411)
🔇 Additional comments (4)
MicrocontrollerCode/include/JoystickFunctions.h (1)
33-38: LGTM!The function declaration and documentation are clear and appropriate.
MicrocontrollerCode/src/main.cpp (1)
107-111: LGTM! Verify that zero displacement is the intended behavior during ADC errors.The
thetaRefvariable is correctly zero-initialized and only populated when the joystick ADC is functioning. Whenjoystick_adc_erroris true, zero displacement values will be transmitted, which appears consistent with the existingomegaRefhandling pattern.Confirm that transmitting zero displacement during ADC errors aligns with the system's error handling strategy and won't cause unexpected behavior in downstream ROS nodes.
MicrocontrollerCode/src/microRosFunctions.cpp (2)
376-378: LGTM! Signature and assignments are correct.The function signature update and displacement field assignments are implemented correctly. The displacement data will be transmitted alongside existing sensor data.
Note: The correctness of transmitted displacement values depends on fixing the
int8_toverflow issue identified inMicrocontrollerCode/include/JoystickFunctions.hand the normalization issue inMicrocontrollerCode/src/JoystickFunctions.cpp.
411-415: LGTM! Debug version correctly mirrors the ROS implementation.The ROS_DEBUG version of
transmitMsgis correctly updated to include displacement data, maintaining consistency with the full ROS implementation.
| struct RefDisplacement { | ||
| int8_t longDisp; ///< Forward/Backward displacement with + indicating forward | ||
| int8_t latDisp; ///< Side to side displacement with + indicating right |
There was a problem hiding this comment.
Critical: int8_t will overflow with raw ADC displacement values.
Based on the implementation in MicrocontrollerCode/src/JoystickFunctions.cpp (lines 29-30), the displacement calculation produces values in the range of approximately -4400 to +4400 (from ADC readings 0-17390 minus offset 12900). Storing these in int8_t fields (range: -128 to 127) will cause severe integer overflow, resulting in incorrect displacement data being transmitted.
Apply this diff to use an appropriate integer type:
struct RefDisplacement {
- int8_t longDisp; ///< Forward/Backward displacement with + indicating forward
- int8_t latDisp; ///< Side to side displacement with + indicating right
+ int16_t longDisp; ///< Forward/Backward displacement with + indicating forward
+ int16_t latDisp; ///< Side to side displacement with + indicating right
};Note: You'll also need to update the corresponding message field types in the ROS message definition to match.
| RefDisplacement joystickToDisplacement(Adafruit_ADS1115 &adc){ | ||
| int forwardJoystick = adc.readADC_SingleEnded(0); //a0 is forward/backward | ||
| int sidewaysJoystick = adc.readADC_SingleEnded(1); //a1 is left/right | ||
|
|
||
| RefDisplacement displacements; | ||
| /* | ||
| * Joystick middle values: ~8500 | ||
| * a0 middle value: ~8500 | ||
| * a1 middle value: ~8300 | ||
| * a0 deadzone 10000 - 6500 | ||
| * a1 deadzone 11000 - 6000 | ||
| * Joystick Min: 0 | ||
| * Joystick Max: 17390 | ||
| * Output is a value -100 to 100 for the speed of the motor | ||
| */ | ||
|
|
||
| //Converting the speeds so they start around 0 and then go positive and negative | ||
| displacements.longDisp = forwardJoystick - (8500+4400); //The second value is used to zero it out when the ADC gain is set to 0 instead of the default (2/3) | ||
| displacements.latDisp = sidewaysJoystick - (8400+4400); | ||
| return displacements; | ||
| } |
There was a problem hiding this comment.
Critical: Integer overflow and misleading comment.
This function has multiple critical issues:
-
Integer overflow: The calculation
forwardJoystick - (8500+4400)produces values in the range of approximately -4400 to +4400 (from ADC readings 0-17390 minus offset 12900), but these are assigned toint8_tfields (range: -128 to 127) in theRefDisplacementstruct. This will cause severe overflow, corrupting the displacement data. -
Misleading comment: Line 25 states "Output is a value -100 to 100 for the speed of the motor" but the function neither scales to this range nor relates to motor speed—it's displacement data. The comment appears to be copy-pasted from
joystickToSpeedwithout updating. -
No normalization: Unlike
joystickToSpeedwhich normalizes and scales to ±100, this function returns raw centered ADC values without any processing.
Apply this diff to normalize displacement to a ±100 range and fix the comment:
RefDisplacement joystickToDisplacement(Adafruit_ADS1115 &adc){
int forwardJoystick = adc.readADC_SingleEnded(0); //a0 is forward/backward
int sidewaysJoystick = adc.readADC_SingleEnded(1); //a1 is left/right
RefDisplacement displacements;
/*
* Joystick middle values: ~8500
* a0 middle value: ~8500
* a1 middle value: ~8300
* a0 deadzone 10000 - 6500
* a1 deadzone 11000 - 6000
* Joystick Min: 0
* Joystick Max: 17390
- * Output is a value -100 to 100 for the speed of the motor
+ * Output is a value -100 to 100 for joystick displacement
*/
- //Converting the speeds so they start around 0 and then go positive and negative
- displacements.longDisp = forwardJoystick - (8500+4400); //The second value is used to zero it out when the ADC gain is set to 0 instead of the default (2/3)
- displacements.latDisp = sidewaysJoystick - (8400+4400);
+ // Center the joystick values around 0
+ forwardJoystick = forwardJoystick - (8500+4400);
+ sidewaysJoystick = sidewaysJoystick - (8400+4400);
+
+ // Normalize to -100 to 100 range
+ const float MAX_INPUT = 13000.0f;
+ float longNorm = constrain(forwardJoystick / MAX_INPUT, -1.0f, 1.0f);
+ float latNorm = constrain(sidewaysJoystick / MAX_INPUT, -1.0f, 1.0f);
+
+ displacements.longDisp = (int8_t)roundf(longNorm * 100.0f);
+ displacements.latDisp = (int8_t)roundf(latNorm * 100.0f);
return displacements;
}Alternative: If you need the full raw ADC range, change the struct fields to int16_t as suggested in the header file review and keep the raw values. Choose based on whether downstream consumers need raw ADC values or normalized joystick position.
📝 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.
| RefDisplacement joystickToDisplacement(Adafruit_ADS1115 &adc){ | |
| int forwardJoystick = adc.readADC_SingleEnded(0); //a0 is forward/backward | |
| int sidewaysJoystick = adc.readADC_SingleEnded(1); //a1 is left/right | |
| RefDisplacement displacements; | |
| /* | |
| * Joystick middle values: ~8500 | |
| * a0 middle value: ~8500 | |
| * a1 middle value: ~8300 | |
| * a0 deadzone 10000 - 6500 | |
| * a1 deadzone 11000 - 6000 | |
| * Joystick Min: 0 | |
| * Joystick Max: 17390 | |
| * Output is a value -100 to 100 for the speed of the motor | |
| */ | |
| //Converting the speeds so they start around 0 and then go positive and negative | |
| displacements.longDisp = forwardJoystick - (8500+4400); //The second value is used to zero it out when the ADC gain is set to 0 instead of the default (2/3) | |
| displacements.latDisp = sidewaysJoystick - (8400+4400); | |
| return displacements; | |
| } | |
| RefDisplacement joystickToDisplacement(Adafruit_ADS1115 &adc){ | |
| int forwardJoystick = adc.readADC_SingleEnded(0); //a0 is forward/backward | |
| int sidewaysJoystick = adc.readADC_SingleEnded(1); //a1 is left/right | |
| RefDisplacement displacements; | |
| /* | |
| * Joystick middle values: ~8500 | |
| * a0 middle value: ~8500 | |
| * a1 middle value: ~8300 | |
| * a0 deadzone 10000 - 6500 | |
| * a1 deadzone 11000 - 6000 | |
| * Joystick Min: 0 | |
| * Joystick Max: 17390 | |
| * Output is a value -100 to 100 for joystick displacement | |
| */ | |
| // Center the joystick values around 0 | |
| forwardJoystick = forwardJoystick - (8500 + 4400); | |
| sidewaysJoystick = sidewaysJoystick - (8400 + 4400); | |
| // Normalize to -100 to 100 range | |
| const float MAX_INPUT = 13000.0f; | |
| float longNorm = constrain(forwardJoystick / MAX_INPUT, -1.0f, 1.0f); | |
| float latNorm = constrain(sidewaysJoystick / MAX_INPUT, -1.0f, 1.0f); | |
| displacements.longDisp = (int8_t)roundf(longNorm * 100.0f); | |
| displacements.latDisp = (int8_t)roundf(latNorm * 100.0f); | |
| return displacements; | |
| } |
|
Note Docstrings generation - SKIPPED |
Docstrings generation was requested by @arturomatlin. * #1 (comment) The following files were modified: * `MicrocontrollerCode/src/JoystickFunctions.cpp` * `MicrocontrollerCode/src/main.cpp` * `MicrocontrollerCode/src/microRosFunctions.cpp`
2 participants
Added joystick logs
Summary by CodeRabbit