Fix carriage detection on the right #205
Conversation
WalkthroughThe changes in this pull request involve modifications to the encoder logic and the knitter's state management within the AYAB firmware. Key updates include the introduction of new conditions in the interrupt service routines for encoder A, particularly for handling the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jonathanperret
force-pushed
the
fix-right-sensor
branch
2 times, most recently
from
e031e83 to
43e53f4
Compare
jonathanperret
changed the title
jonathanperret
force-pushed
the
fix-right-sensor
branch
from
43e53f4 to
35ea8fb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
test/test_encoders.cpp (2)
134-161: LGTM! Consider adding test case documentation.The test case correctly validates the sequence of carriage detection transitions from Lace to Garter, including both rising and falling edge scenarios. This aligns well with the PR's objective of improving right-side carriage detection.
Consider adding a brief comment explaining the expected behavior sequence, as this test case covers a complex state transition:
+// Test case verifies Garter carriage detection through a sequence of: +// 1. Initial rising edge (detecting Lace carriage) +// 2. Falling edge validation +// 3. Second rising edge (confirming Garter carriage) TEST_F(EncodersTest, test_encA_rising_in_front_G_carriage) {
223-230: LGTM! Consider additional edge case validation.The test case correctly validates the special case handling for KH910's inverted K-only reading, as mentioned in the PR objectives.
Consider adding test cases for edge conditions:
+// Test invalid hall sensor values +TEST_F(EncodersTest, test_encA_falling_invalid_hall_values_KH910) { + encoders->init(Machine_t::Kh910); + + // Test with hall sensor value at exactly FILTER_R_MIN + EXPECT_CALL(*arduinoMock, digitalRead(ENC_PIN_A)).WillOnce(Return(false)); + EXPECT_CALL(*arduinoMock, digitalRead(ENC_PIN_B)); + EXPECT_CALL(*arduinoMock, analogRead(EOL_PIN_R)) + .WillOnce(Return(FILTER_R_MIN[static_cast<int8_t>(encoders->getMachineType())])); + + encoders->encA_interrupt(); + ASSERT_EQ(encoders->getCarriage(), Carriage_t::NoCarriage); +}src/ayab/knitter.cpp (1)
220-225: LGTM: Early Garter carriage initializationThe documentation clearly explains the need for early initialization with Garter carriages, and the implementation correctly allows for solenoid setting before the carriage center crosses the turn mark.
Consider moving the Garter carriage check before the hall sensor checks for better performance, as it could avoid unnecessary calculations when a Garter carriage is detected:
- bool passedLeft = (Direction_t::Right == m_direction) && (Direction_t::Left == m_lastHall) && - (m_position > (END_LEFT_PLUS_OFFSET[static_cast<uint8_t>(m_machineType)] + GARTER_SLOP)); - bool passedRight = (Direction_t::Left == m_direction) && (Direction_t::Right == m_lastHall) && - (m_position < (END_RIGHT_MINUS_OFFSET[static_cast<uint8_t>(m_machineType)] - GARTER_SLOP)); - - if (passedLeft || passedRight || m_carriage == Carriage::Garter) { + if (m_carriage == Carriage::Garter) { + return true; + } + + bool passedLeft = (Direction_t::Right == m_direction) && (Direction_t::Left == m_lastHall) && + (m_position > (END_LEFT_PLUS_OFFSET[static_cast<uint8_t>(m_machineType)] + GARTER_SLOP)); + bool passedRight = (Direction_t::Left == m_direction) && (Direction_t::Right == m_lastHall) && + (m_position < (END_RIGHT_MINUS_OFFSET[static_cast<uint8_t>(m_machineType)] - GARTER_SLOP)); + + if (passedLeft || passedRight) {
🛑 Comments failed to post (3)
src/ayab/encoders.cpp (3)
191-193:
⚠️ Potential issueFix enum type usage: Use Carriage_t instead of Carriage
There's a syntax error in the enum comparison. The code uses
Carriage::but the enum type isCarriage_t::.Apply this fix:
- } else if (m_carriage == Carriage::Lace && - detected_carriage == Carriage::Knit && + } else if (m_carriage == Carriage_t::Lace && + detected_carriage == Carriage_t::Knit &&📝 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.} else if (m_carriage == Carriage_t::Lace && detected_carriage == Carriage_t::Knit && m_position > start_position) {
241-244:
⚠️ Potential issueFix enum type usage and enhance comment
Two issues to address:
- Incorrect enum type usage (
MachineType::instead ofMachine_t::)- Comment could be more specific about future support plans
Apply this fix:
- if (m_machineType == MachineType::Kh270) { + if (m_machineType == Machine_t::Kh270) {Consider enhancing the comment to be more specific about future support plans:
- // No attempt to support KH270 on the right for now as I can't test it + // TODO: Add KH270 right side support after hardware testing capability is available📝 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.// TODO: Add KH270 right side support after hardware testing capability is available if (m_machineType == Machine_t::Kh270) { return; }
272-281:
⚠️ Potential issueFix enum type and consider hardware fix for KH910
The code contains two issues:
- Incorrect enum type usage (
MachineType::instead ofMachine_t::)- Hardware limitation requiring software workaround
Apply this fix:
- if (m_machineType == MachineType::Kh910) { + if (m_machineType == Machine_t::Kh910) {The current workaround for KH910's wiring issue, while functional, might be better addressed through a hardware fix. Consider:
- Documenting the exact wiring issue in the codebase
- Creating a hardware fix proposal
- Adding this to the project's roadmap for a more permanent solution
📝 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.if (m_machineType == Machine_t::Kh910) { // Due to an error in wiring on the shield, the sensor only triggers for the K carriage, // and with a low voltage so we can't use the same logic as for other machines. detected_carriage = Carriage_t::Knit; } else { if (hallValue >= FILTER_R_MIN[static_cast<uint8_t>(m_machineType)]) { detected_carriage = Carriage_t::Knit; } else { detected_carriage = Carriage_t::Lace; }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/ayab/knitter.cpp (1)
Line range hint
214-219: Document GARTER_SLOP constantThe
GARTER_SLOPconstant is used in position calculations but its purpose and value determination aren't documented. Consider:
- Adding a comment explaining what this constant represents
- Documenting how the value was determined
- Moving it to a named constant with clear documentation if it's defined elsewhere
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/ayab/knitter.cpp(2 hunks)
🔇 Additional comments (2)
src/ayab/knitter.cpp (2)
79-79: LGTM: Good addition of carriage state tracking
The new member variable m_carriage properly encapsulates the carriage state and follows good initialization practices.
220-225: Verify carriage type stability assumption
The comment suggests that once a Garter carriage is detected, the carriage type won't change. Let's verify this assumption across the codebase.
✅ Verification successful
Carriage type detection is stable and correctly implemented
The verification shows that the carriage type detection is implemented safely:
- The Garter carriage is only detected through a specific sequence: first detecting Lace followed by Knit pattern in a specific position (due to magnet placement)
- Once detected as Garter, there's no code path that changes the carriage type back to another type
- The carriage type is only modified in two scenarios:
- Initial detection when m_carriage is NoCarriage
- Specific Garter carriage detection sequence
The assumption in the code is valid - once a Garter carriage is detected, the type remains stable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if carriage type can change after Garter detection
# Search for places where carriage type is modified
echo "Searching for carriage type modifications..."
rg -A 5 "Carriage::(Garter|NoCarriage)" --type cpp
# Search for relevant test cases
echo "Searching for related test cases..."
rg -A 5 "TEST.*Garter.*carriage" --type cpp
Length of output: 911
Script:
#!/bin/bash
# Let's search for all places where m_carriage is assigned or modified
echo "Searching for m_carriage assignments..."
rg -A 5 "m_carriage\s*=" --type cpp
# Let's also look for the Carriage enum definition
echo "Searching for Carriage enum definition..."
rg -B 5 -A 5 "enum\s+class\s+Carriage" --type cpp
# Search for carriage detection logic
echo "Searching for carriage detection logic..."
rg -B 5 "setCarriage|detectCarriage" --type cpp
Length of output: 5903
jonathanperret
force-pushed
the
fix-right-sensor
branch
from
35ea8fb to
1c9a4af
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/ayab/encoders.cpp(4 hunks)src/ayab/encoders.h(2 hunks)src/ayab/knitter.cpp(2 hunks)test/test_encoders.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ayab/encoders.h
- test/test_encoders.cpp
🔇 Additional comments (3)
src/ayab/encoders.cpp (2)
153-158: LGTM: Early return for Garter carriage prevents state corruption.
The early return when detecting a Garter carriage is a good defensive measure to prevent the second set of magnets from incorrectly resetting the state.
284-298:
Fix enum types and verify position adjustment logic.
- The code uses incorrect enum types (
Carriage::instead ofCarriage_t::). - The position adjustment for the Garter carriage is well-documented but should be verified.
Apply this fix for the enum types:
- if (m_carriage == Carriage::Lace &&
- detected_carriage == Carriage::Knit &&
+ if (m_carriage == Carriage_t::Lace &&
+ detected_carriage == Carriage_t::Knit &&Let's verify the position adjustment logic:
✅ Verification successful
Let me gather more context about the Garter carriage detection logic.
Based on the gathered information, I can now generate the final response:
Fix enum types and position adjustment logic is verified.
The position adjustment logic for the Garter carriage is well-documented and correct:
- The
GARTER_L_MAGNET_SPACINGconstant (24U) is used to maintain consistency in carriage position detection - The code is properly handling the asymmetric magnet detection between left and right sides
- The adjustment is necessary because the Garter carriage has two sets of magnets, and we need consistent position reporting regardless of which direction the carriage approaches from
Apply this fix for the enum types:
- if (m_carriage == Carriage::Lace &&
- detected_carriage == Carriage::Knit &&
+ if (m_carriage == Carriage_t::Lace &&
+ detected_carriage == Carriage_t::Knit &&🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GARTER_L_MAGNET_SPACING constant matches the physical measurements
# Search for the constant definition and its usage
rg "GARTER_L_MAGNET_SPACING" -A 2
Length of output: 364
Script:
#!/bin/bash
# Search for documentation or comments about Garter carriage magnet spacing
rg -B 2 -A 2 "Garter.*magnet" --ignore-case
# Look for related constants and their usage
rg "NUM_CARRIAGES|Carriage_t::(Garter|Lace|Knit)" -B 1 -A 1
# Find the enum definition
ast-grep --pattern 'enum $_ {
$$$
Garter
$$$
}'
Length of output: 10606
src/ayab/knitter.cpp (1)
220-225: LGTM: Well-documented Garter carriage initialization.
The modification to allow early initialization for Garter carriages is logical and well-explained. The comment clearly documents why we need to start setting solenoids before the carriage center crosses the turn mark.
Mostly by copying the logic from encA_rising into encA_falling, and special-casing the KH910 with its inverted K-only reading. Also, if we have detected a Garter carriage, we already decided that it's too hard to try to reset the position when it crosses the turn marks. If we punt on resetting the belt shift as well, we save some logic and fix the few weird spots where it turns while its outer magnet is right on the turn mark.
The "pass turn mark" test is not appropriate for the Garter carriage.
jonathanperret
force-pushed
the
fix-right-sensor
branch
from
1c9a4af to
79cd0eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/ayab/knitter.cpp (1)
220-225: Consider separating carriage detection from state managementThe current implementation tightly couples carriage detection with the machine's ready state logic. Consider extracting the carriage detection logic into a separate method or class to:
- Improve testability of carriage detection logic
- Make it easier to add support for new carriage types
- Separate concerns between state management and carriage detection
Example structure:
class CarriageDetector { bool isCarriageReady(Carriage_t type, Direction_t dir, uint16_t pos) { switch(type) { case Carriage_t::Garter: return true; // Immediate readiness default: return checkPositionBasedReadiness(dir, pos); } } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/ayab/encoders.cpp(4 hunks)src/ayab/encoders.h(2 hunks)src/ayab/knitter.cpp(2 hunks)test/test_encoders.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ayab/encoders.h
- src/ayab/encoders.cpp
- test/test_encoders.cpp
🔇 Additional comments (2)
src/ayab/knitter.cpp (2)
79-79: LGTM: Proper initialization of new carriage state member
The initialization of m_carriage follows the established pattern and uses the correct enum type.
220-225: Verify safety of early solenoid activation with Garter carriage
While the early detection of the Garter carriage makes sense for improving detection, the comment indicates that solenoids may be activated before the carriage center crosses the turn mark. This could potentially lead to unsafe or incorrect needle operations.
Please verify:
- That solenoid activation timing is properly coordinated with carriage position
- That bypassing position checks (
passedLeftandpassedRight) for the Garter carriage doesn't introduce edge cases
Let's verify the solenoid activation logic for Garter carriage:
✅ Verification successful
Let me gather more context about the solenoid activation timing and position checks.
Based on the code analysis, I can now generate the final response:
Early solenoid activation for Garter carriage is safely implemented
The implementation has proper safety checks in place:
- Solenoid activation is controlled by
calculatePixelAndSolenoid()which verifies position and direction before activating any solenoids - The Garter carriage detection includes specific position checks and magnet spacing adjustments (
GARTER_L_MAGNET_SPACING) - The early detection is balanced with
GARTER_SLOPtolerance to prevent premature activation - Test cases verify that solenoids are not activated when position calculations are invalid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for solenoid activation logic specific to Garter carriage
rg -l "Garter" | xargs rg "setSolenoid|SOLENOID" -B 5 -A 5
# Look for safety checks around solenoid activation
ast-grep --pattern 'setSolenoid($$$)'
Length of output: 12563
Script:
#!/bin/bash
# Look for carriage position calculations and checks
rg "m_position|calculatePixelAndSolenoid" -B 3 -A 3
# Search for Garter carriage specific logic
ast-grep --pattern 'if ($_ == Carriage_t::Garter) {
$$$
}'
Length of output: 10596
Problem
Fixes carriages not being detected on the right on KH930-like machines (fixes #175, fixes #176, "fixes" #215 by disabling the currently broken right-side sensor support for KH270).
Proposed solution
On the KH910/KH950, we can't fix detecting Lace or Garter carriages on the right without an AYAB hardware change. But on other machines, the right sensor is just as functional as the left one.
So in this PR, we mostly copy the logic from
encA_risingintoencA_falling, and add a special case for the KH910 with its inverted K-only reading.We also have to change the initialization logic (in
knitter.cpp) slightly: the "carriage passed the turn mark" test does not work for the Garter carriage, because the position we track is actually that of its rightmost magnet pair. It turns out we can declare the machine ready as soon as a Garter carriage has been detected, since that means the carriage type cannot change further.How to test
A firmware built from this PR can we flashed at this URL: https://code.jonathanperret.net/ayab-webtool/#pr=205
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests