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 initialization

The 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) {

src/ayab/encoders.cpp (3)

191-193: ⚠️ Potential issue

Fix 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 is Carriage_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 issue

Fix enum type usage and enhance comment

Two issues to address:

1. Incorrect enum type usage (MachineType:: instead of Machine_t::)
2. 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 issue

Fix enum type and consider hardware fix for KH910

The code contains two issues:

1. Incorrect enum type usage (MachineType:: instead of Machine_t::)
2. 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:

1. Documenting the exact wiring issue in the codebase
2. Creating a hardware fix proposal
3. 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;
      }

src/ayab/knitter.cpp (1)

Line range hint 214-219: Document GARTER_SLOP constant

The GARTER_SLOP constant is used in position calculations but its purpose and value determination aren't documented. Consider:

1. Adding a comment explaining what this constant represents
2. Documenting how the value was determined
3. Moving it to a named constant with clear documentation if it's defined elsewhere

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/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

#!/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

Configuration used: CodeRabbit UI
Review profile: CHILL

-    if (m_carriage == Carriage::Lace &&
-               detected_carriage == Carriage::Knit &&
+    if (m_carriage == Carriage_t::Lace &&
+               detected_carriage == Carriage_t::Knit &&

-    if (m_carriage == Carriage::Lace &&
-               detected_carriage == Carriage::Knit &&
+    if (m_carriage == Carriage_t::Lace &&
+               detected_carriage == Carriage_t::Knit &&

#!/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

#!/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
  $$$
}'

src/ayab/knitter.cpp (1)

220-225: Consider separating carriage detection from state management

The 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:

1. Improve testability of carriage detection logic
2. Make it easier to add support for new carriage types
3. 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);
    }
  }
};

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/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($$$)'

#!/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) {
  $$$
}'