Systemic changes to support garter carriage on the full bed #196
Conversation
WalkthroughThe recent changes enhance the AYAB firmware by implementing an error signaling mechanism across the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GlobalBeeper
participant Beeper
User->>GlobalBeeper: call error()
GlobalBeeper->>Beeper: invoke m_instance.error()
Beeper->>Beeper: check m_enabled
Beeper->>Beeper: trigger beep with BEEP_NUM_ERROR
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/ayab/encoders.cpp (3 hunks)
- src/ayab/encoders.h (2 hunks)
Additional comments not posted (6)
src/ayab/encoders.h (2)
86-87: Adjustments toSTART_OFFSETarray for KH910 machine configuration.The changes to the
START_OFFSETarray modify the initial offsets for the Left and Right directions for the Garter carriage. Ensure that these adjustments have been tested and validated to confirm their impact on the encoder logic.
114-114: Addition ofHALF_MAGNET_DISTANCE_Gconstant.The new constant
HALF_MAGNET_DISTANCE_Gwith a value of12Usuggests a potential enhancement or new feature related to magnet distance calculations. Ensure that this constant is used consistently and correctly in the codebase.Verification successful
Addition of
HALF_MAGNET_DISTANCE_Gconstant.The new constant
HALF_MAGNET_DISTANCE_Gwith a value of12Uis used consistently and correctly in the codebase, specifically within the logic that adjusts thestart_positionbased on the detected carriage type and position.
src/ayab/encoders.cpp: Used in a conditional block to adjuststart_position.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `HALF_MAGNET_DISTANCE_G` constant in the codebase. # Test: Search for the usage of the constant. Expect: Consistent and correct usage. rg --type cpp 'HALF_MAGNET_DISTANCE_G'Length of output: 192
Script:
#!/bin/bash # Description: Extract surrounding lines of code where `HALF_MAGNET_DISTANCE_G` is used in `encoders.cpp` to understand its context. # Extract 5 lines before and after the usage of `HALF_MAGNET_DISTANCE_G` in `encoders.cpp` rg --type cpp 'HALF_MAGNET_DISTANCE_G' -A 5 -B 5Length of output: 1175
Script:
#!/bin/bash # Description: Extract definitions and usages of `start_position`, `m_carriage`, and `detected_carriage` in `encoders.cpp`. # Search for the definitions and usages of `start_position` rg --type cpp 'start_position' -A 5 -B 5 # Search for the definitions and usages of `m_carriage` rg --type cpp 'm_carriage' -A 5 -B 5 # Search for the definitions and usages of `detected_carriage` rg --type cpp 'detected_carriage' -A 5 -B 5Length of output: 15881
src/ayab/encoders.cpp (4)
136-137: Simplified condition for updatingm_positioninencA_risingmethod.The condition for updating
m_positionhas been simplified by removing the upper boundary check againstEND_RIGHT. Ensure that this change has been tested and validated to confirm its impact on the encoder logic.
179-180: Altered handling ofstart_positioninencA_risingmethod.The handling of
start_positionhas been altered by replacing the return statement with a decrement operation. Ensure that this change has been tested and validated to confirm its impact on the encoder logic.
205-207: Simplified condition for updatingm_positioninencA_fallingmethod.The condition for updating
m_positionhas been simplified by removing the lower boundary check againstEND_LEFT. Ensure that this change has been tested and validated to confirm its impact on the encoder logic.
208-213: New condition to return early if carriage type isGarterinencA_fallingmethod.A new condition has been introduced to return early if the carriage type is
Garter, preventing further processing. Ensure that this change has been tested and validated to confirm its impact on the encoder logic.
|
First, congratulations on isolating this bug, it must not have been easy! The following question is asked out of curiosity, feel free to disregard it.
I feel like I'm missing something here. I agree that Likewise when overflowing, if Compiler explorer seems to show that the same assembly code is generated with or without the Did you observe different behavior when adding the Finally, as suggested by @X-sam on Discord, wouldn't it be safer all around to upgrade |
Just thought of a potential downside to making |
|
Not ready for review, still sorting out RTL offsets and then need to do a lot of cleanup. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
src/ayab/encoders.h (2)
114-114: Unused constantHALF_MAGNET_DISTANCE_G.The constant
HALF_MAGNET_DISTANCE_Gis defined but not used anywhere in the codebase. Consider reviewing its intended purpose and ensure it is integrated where necessary. If it is not needed, consider removing it to maintain code clarity and cleanliness.
- Location:
src/ayab/encoders.hat line 114Analysis chain
Addition of
HALF_MAGNET_DISTANCE_G.The new constant
HALF_MAGNET_DISTANCE_Ghas been introduced. Verify its usage in the codebase to ensure it is correctly integrated and serves its intended purpose.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `HALF_MAGNET_DISTANCE_G` to ensure it's correctly integrated. # Test: Search for the constant usage. Expect: Proper integration and usage. rg --type cpp --word-regexp 'HALF_MAGNET_DISTANCE_G'Length of output: 120
80-80: Potential Type Mismatch withGARTER_SLOPUsageThe change of
GARTER_SLOPtouint16_tmay cause issues when used in arithmetic operations withuint8_tvariables, as seen in:
test/test_fsm.cpp: Assignments touint8_tvariablespositionPassedLeftandpositionPassedRight.test/test_knitter.cpp: Return values involvingGARTER_SLOP.src/ayab/knitter.cpp: Conditional checks involvingGARTER_SLOP.Ensure that these operations do not result in values exceeding the
uint8_trange, or consider adjusting the types of the variables involved to prevent truncation.Analysis chain
Type Change for
GARTER_SLOP.The change from
uint8_ttouint16_tforGARTER_SLOPallows for larger values, which may be necessary for accommodating larger offsets or precision. Ensure that all usages ofGARTER_SLOPare compatible with this type change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `GARTER_SLOP` are compatible with `uint16_t`. # Test: Search for the constant usage. Expect: No type mismatch errors. rg --type cpp --word-regexp 'GARTER_SLOP'Length of output: 863
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/ayab/encoders.cpp (5 hunks)
- src/ayab/encoders.h (2 hunks)
- src/ayab/knitter.cpp (3 hunks)
Additional comments not posted (8)
src/ayab/encoders.h (1)
86-87: Modification ofSTART_OFFSETValues.The
START_OFFSETarray values for the KH910 machine have been adjusted. Ensure these changes align with the intended machine configuration and verify that they do not introduce any inconsistencies in the logic where they are used.Verification successful
Modification of
START_OFFSETValues Verified.The changes to the
START_OFFSETvalues for the KH910 machine have been tested in various scenarios withintest/test_knitter.cpp. These tests cover different directions and machine configurations, ensuring the modifications align with the intended logic without introducing inconsistencies.
- Test cases in
test/test_knitter.cppverify the correct behavior ofgetStartOffsetunder different conditions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `START_OFFSET` to ensure consistency with the new values. # Test: Search for the array usage. Expect: No logic errors due to value changes. rg --type cpp --word-regexp 'START_OFFSET'Length of output: 290
Script:
#!/bin/bash # Search for the definition and usage of m_machineType, direction, and m_carriage rg --type cpp 'm_machineType|direction|m_carriage' -A 5Length of output: 26943
Script:
#!/bin/bash # Search for test cases involving getStartOffset or START_OFFSET in test files rg --type cpp 'getStartOffset|START_OFFSET' test/test_knitter.cpp -A 5Length of output: 2830
src/ayab/encoders.cpp (4)
222-223: ```shell
#!/bin/bashUse ripgrep to search for the
encA_fallingfunction definition in the codebase.rg --type cpp --word-regexp 'void encA_falling'
--- `146-147`: ```shell #!/bin/bash # Extract the implementation of the `encA_rising` function from `src/ayab/encoders.cpp`. ast-grep --lang cpp --pattern 'void Encoders::encA_rising() { $$$ }'
206-207: Simplification of Position Update Logic inencA_falling.The position update logic has been simplified by removing boundary checks and directly decrementing
m_position. Ensure this change does not introduce errors in scenarios where boundary checks were previously necessary.
136-137: Simplification of Position Update Logic inencA_rising.The position update logic has been simplified by removing boundary checks and directly incrementing
m_position. Ensure this change does not introduce errors in scenarios where boundary checks were previously necessary.src/ayab/knitter.cpp (3)
274-274: Commented OutGlobalBeeper::finishedLineinknit.The invocation of
GlobalBeeper::finishedLinebased onm_positionhas been commented out. Verify that this change does not affect the intended functionality and that the new conditions for calling this function are correct.#!/bin/bash # Description: Verify the impact of commenting out `GlobalBeeper::finishedLine` in `knit`. # Test: Search for all `GlobalBeeper::finishedLine` invocations. Expect: Correct conditions for invocation. rg --type cpp --word-regexp 'GlobalBeeper::finishedLine'
286-292: New Conditions forGlobalBeeper::finishedLineinknit.The function is now called when
m_pixelToSetequals 0 or 200. Ensure these conditions are correct and align with the intended functionality.#!/bin/bash # Description: Verify the correctness of the new conditions for `GlobalBeeper::finishedLine` in `knit`. # Test: Search for logic related to `m_pixelToSet`. Expect: Correct conditions for invocation. rg --type cpp --word-regexp 'm_pixelToSet'
431-437: Modification of Logic incalculatePixelAndSolenoid.The conditions for calculating
m_solenoidToSethave been adjusted. Ensure these changes maintain the intended functionality and do not introduce errors.#!/bin/bash # Description: Verify the impact of changes in `calculatePixelAndSolenoid`. # Test: Search for logic related to `m_solenoidToSet`. Expect: Correct calculation and no errors. rg --type cpp --word-regexp 'm_solenoidToSet'
clholgat
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/ayab/encoders.cpp (6 hunks)
- src/ayab/encoders.h (3 hunks)
- src/ayab/knitter.cpp (3 hunks)
Additional comments not posted (14)
src/ayab/encoders.h (6)
77-77: LGTM!The constant
ALL_MAGNETS_CLEARED_LEFTis correctly defined and initialized.The code changes are approved.
78-78: LGTM!The constant
ALL_MAGNETS_CLEARED_RIGHTis correctly defined and initialized.The code changes are approved.
83-83: LGTM!Changing the type of
GARTER_SLOPfromuint8_ttouint16_tis appropriate to accommodate a larger range of values.The code changes are approved.
89-90: LGTM!The updated values in the
START_OFFSETarray reflect new configurations for the left and right offsets, suggesting a recalibration of the machine's operational parameters.The code changes are approved.
117-117: LGTM!The constant
HALF_MAGNET_DISTANCE_Gis correctly defined and initialized.The code changes are approved.
188-189: LGTM!The member variables
m_passedLeftandm_passedRightare correctly defined and initialized.The code changes are approved.
src/ayab/encoders.cpp (3)
78-79: LGTM!The member variables
m_passedLeftandm_passedRightare correctly initialized in theinitmethod.The code changes are approved.
138-144: LGTM!The logic for updating
m_positionandm_passedLeftin theencA_risingmethod is streamlined and ensures that the state is only reset when appropriate.The code changes are approved.
218-224: LGTM!The logic for updating
m_positionandm_passedRightin theencA_fallingmethod is streamlined and ensures that the state is only reset when appropriate.The code changes are approved.
src/ayab/knitter.cpp (5)
274-274: LGTM!The addition of
GlobalBeeper::finishedLine()in theknitmethod indicates a modification in the control flow that may affect the feedback mechanism of the knitting process.The code changes are approved.
349-349: LGTM!Commenting out the call to
GlobalBeeper::finishedLine()in thesetNextLinemethod alters the behavior of the method by potentially suppressing feedback when no new line is requested.The code changes are approved.
401-405: LGTM!The new implementation in the
calculatePixelAndSolenoidmethod directly calculatesm_pixelToSetbased on thestartOffsetand modifies howm_solenoidToSetis determined based on theBeltShiftstate.The code changes are approved.
412-417: LGTM!The new implementation in the
calculatePixelAndSolenoidmethod directly calculatesm_pixelToSetbased on thestartOffsetand modifies howm_solenoidToSetis determined based on theBeltShiftstate.The code changes are approved.
349-349: LGTM!Commenting out the call to
GlobalBeeper::finishedLine()in thesetNextLinemethod alters the behavior of the method by potentially suppressing feedback when no new line is requested.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/ayab/encoders.h (2 hunks)
- src/ayab/knitter.cpp (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/ayab/encoders.h
- src/ayab/knitter.cpp
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
src/ayab/knitter.cpp (2)
407-409: Remove commented-out code if no longer needed.The commented-out code related to the Knit carriage should be removed if it is no longer needed to keep the codebase clean.
- //if (Carriage_t::Knit == m_carriage) { - // beltShift = !beltShift; - //}
405-406: Replace magic numbers with named constants.The magic numbers should be replaced with named constants for better readability and maintainability.
- // See page 7 of the 930 service manual https://mkmanuals.com/downloadable/download/sample/sample_id/27/ - // It's probably more apt to say that the other carriages are special but the + // See page 7 of the 930 service manual https://mkmanuals.com/downloadable/download/sample/sample_id/27/ + // It's probably more apt to say that the other carriages are special but the + // Named constants for magic numbers + constexpr uint8_t KNIT_CARRIAGE_PAGE = 7; + constexpr uint8_t KH270_SOLENOID_SHIFT = 3;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/ayab/beeper.cpp (1 hunks)
- src/ayab/beeper.h (4 hunks)
- src/ayab/encoders.cpp (6 hunks)
- src/ayab/encoders.h (2 hunks)
- src/ayab/global_beeper.cpp (1 hunks)
- src/ayab/knitter.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/ayab/encoders.cpp
- src/ayab/encoders.h
Additional comments not posted (7)
src/ayab/global_beeper.cpp (1)
50-52: LGTM!The method correctly delegates the error handling to
m_instance.The code changes are approved.
src/ayab/beeper.cpp (1)
80-84: LGTM!The method correctly implements the error signaling functionality.
The code changes are approved.
src/ayab/beeper.h (4)
36-36: LGTM!The constant is correctly defined and follows the naming convention of other constants in the file.
The code changes are approved.
53-53: LGTM!The method is correctly added as a pure virtual function, ensuring that any implementing class must provide an implementation for handling error beeps.
The code changes are approved.
77-77: LGTM!The method is correctly added to the class, providing a static method for error handling.
The code changes are approved.
92-92: LGTM!The method is correctly added to the class, providing an implementation for handling error beeps.
The code changes are approved.
src/ayab/knitter.cpp (1)
273-274: Good addition of error handling.The addition of
GlobalBeeper::error()provides useful feedback in case of an error.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
src/ayab/encoders.cpp (5)
Line range hint
139-218: Refactor suggestion: Simplify nested if-else structure.The nested if-else structure can be simplified for better readability. Consider refactoring the code as follows:
if (Direction_t::Right == m_direction) { m_position = m_position + (uint8_t) 1; if (m_position > ALL_MAGNETS_CLEARED_LEFT[static_cast<uint8_t>(m_machineType)]) { m_passedLeft = false; } } uint16_t hallValue = analogRead(EOL_PIN_L); if ((hallValue < FILTER_L_MIN[static_cast<uint8_t>(m_machineType)]) || (hallValue > FILTER_L_MAX[static_cast<uint8_t>(m_machineType)])) { m_hallActive = Direction_t::Left; if (!m_passedLeft && Direction_t::Right == m_direction) { m_beltShift = digitalRead(ENC_PIN_C) != 0 ? BeltShift::Shifted : BeltShift::Regular; m_passedLeft = true; if (Carriage_t::Garter == m_carriage && m_position < 30) { m_beltShift = (BeltShift::Regular == m_beltShift) ? BeltShift::Shifted : BeltShift::Regular; } } if (m_carriage == Carriage_t::Garter || (m_carriage == Carriage_t::Knit && m_machineType == Machine_t::Kh270)) { return; } Carriage detected_carriage = (hallValue >= FILTER_L_MIN[static_cast<uint8_t>(m_machineType)]) ? Carriage_t::Knit : Carriage_t::Lace; uint8_t start_position = END_LEFT_PLUS_OFFSET[static_cast<uint8_t>(m_machineType)]; if (m_machineType == Machine_t::Kh270) { m_carriage = Carriage_t::Knit; if (detected_carriage == Carriage_t::Knit) { start_position += MAGNET_DISTANCE_270; } } else if (m_carriage == Carriage_t::NoCarriage) { m_carriage = detected_carriage; } else if (m_carriage != detected_carriage && m_position > start_position) { m_carriage = Carriage_t::Garter; m_beltShift = (BeltShift::Regular == m_beltShift) ? BeltShift::Shifted : BeltShift::Regular; return; } else { m_carriage = detected_carriage; } m_position = start_position; }
Line range hint
243-283: Refactor suggestion: Simplify nested if-else structure.The nested if-else structure can be simplified for better readability. Consider refactoring the code as follows:
if (Direction_t::Left == m_direction) { m_position = m_position - (uint8_t) 1; if (m_position < ALL_MAGNETS_CLEARED_RIGHT[static_cast<uint8_t>(m_machineType)]) { m_passedRight = false; } } uint16_t hallValue = analogRead(EOL_PIN_R); bool hallValueSmall = (hallValue < FILTER_R_MIN[static_cast<uint8_t>(m_machineType)]); if (hallValueSmall || hallValue > FILTER_R_MAX[static_cast<uint8_t>(m_machineType)]) { m_hallActive = Direction_t::Right; if (!m_passedRight && Direction_t::Left == m_direction) { m_beltShift = digitalRead(ENC_PIN_C) != 0 ? BeltShift::Regular : BeltShift::Shifted; m_passedRight = true; } if (m_carriage == Carriage_t::Garter) { return; } if (hallValueSmall) { m_carriage = Carriage_t::Knit; } m_position = END_RIGHT_MINUS_OFFSET[static_cast<uint8_t>(m_machineType)]; }
Line range hint
139-218: Suggestion: Add comments to explain complex logic.The method contains several nested conditions and assignments. Adding comments to explain the logic will improve readability and maintainability.
Line range hint
243-283: Suggestion: Add comments to explain complex logic.The method contains several nested conditions and assignments. Adding comments to explain the logic will improve readability and maintainability.
Line range hint
139-218: Performance optimization: Minimize time spent in ISR.Reading from analog pins and performing multiple conditional checks in an interrupt service routine (ISR) can impact performance. Consider optimizing the code to minimize the time spent in the ISR.
Also applies to: 243-283
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/ayab/encoders.cpp (6 hunks)
- src/ayab/encoders.h (3 hunks)
- src/ayab/knitter.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/ayab/encoders.h
- src/ayab/knitter.cpp
Additional comments not posted (1)
src/ayab/encoders.cpp (1)
74-80: LGTM!The initialization of new member variables
m_passedLeftandm_passedRightis correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/ayab/encoders.cpp (6 hunks)
- src/ayab/encoders.h (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/ayab/encoders.h
Additional comments not posted (5)
src/ayab/encoders.cpp (5)
78-79: LGTM!The initialization of
m_passedLeftandm_passedRighttofalseis correct.
138-144: LGTM!The changes in the
encA_risingmethod enhance the robustness of the encoder's functionality by ensuring accurate tracking of belt shift and carriage state.Also applies to: 153-217
242-248: LGTM!The changes in the
encA_fallingmethod enhance the robustness of the encoder's functionality by ensuring accurate tracking of belt shift and carriage state.Also applies to: 263-277, 280-282
153-171: LGTM!The logic for setting the belt shift signal is correct and ensures that the signal is only set once per turn mark crossing.
263-271: LGTM!The logic for setting the belt shift signal is correct and ensures that the signal is only set once per turn mark crossing.
dl1com
force-pushed
the
clholgat/garter_carriage
branch
from
b802f75 to
b309e39
Compare
Co-authored-by: Jonathan Perret <[email protected]>
Co-authored-by: Jonathan Perret <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/ayab/beeper.cpp (1 hunks)
- src/ayab/beeper.h (4 hunks)
- src/ayab/encoders.cpp (6 hunks)
- src/ayab/encoders.h (3 hunks)
- src/ayab/global_beeper.cpp (1 hunks)
- src/ayab/knitter.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/ayab/beeper.cpp
- src/ayab/encoders.cpp
- src/ayab/encoders.h
- src/ayab/knitter.cpp
Additional comments not posted (5)
src/ayab/global_beeper.cpp (1)
50-52: Approved: Newerror()method inGlobalBeeper.The addition of the
error()method in theGlobalBeeperclass is well-implemented. It correctly delegates the error handling to them_instancemember, which is expected to be an instance of a class implementing theBeeperInterface. This change aligns with the PR's goal to enhance error management capabilities in the firmware.src/ayab/beeper.h (4)
36-36: Approved: New constantBEEP_NUM_ERROR.The definition of
BEEP_NUM_ERRORwith a value of 15 is a clear and straightforward way to standardize the number of beeps for error conditions across the system. This constant should be used consistently wherever error beeps are handled to maintain uniformity.
53-53: Approved: New pure virtualerror()method inBeeperInterface.Adding a pure virtual
error()method to theBeeperInterfaceensures that all implementing classes must provide their own implementation of error handling. This is a good design choice that enforces consistency and makes the error handling mechanism extensible.
77-77: Approved: New staticerror()method inGlobalBeeper.The addition of a static
error()method to theGlobalBeeperclass allows for easy access to error signaling functionality from anywhere in the code that has access toGlobalBeeper. This enhances the modularity and accessibility of the error handling.
92-92: Approved: Implementation oferror()method inBeeper.The implementation of the
error()method in theBeeperclass, which finalizes the method from theBeeperInterface, is crucial for actual error signaling. This method should correctly handle the error beeping logic, potentially utilizing theBEEP_NUM_ERRORconstant for the number of beeps.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
test/test_encoders.cpp (1)
127-127: Inconsistency in Belt Shift Handling for KH270 Machine TypeThere is a discrepancy in the expected belt shift behavior for the KH270 machine type. The test case in
test/test_encoders.cppexpectsBeltShift::Shifted, whiletest/test_knitter.cppexpectsBeltShift::Regular. This inconsistency should be reviewed and resolved to ensure consistent behavior across the firmware.
test/test_encoders.cpp: ExpectsBeltShift::Shiftedtest/test_knitter.cpp: ExpectsBeltShift::RegularAnalysis chain
Updated Assertion for Belt Shift in KH270 Context
The assertion update in
test_encA_rising_in_front_KH270also changes the expected belt shift toBeltShift::Shifted. This specific test case targets the KH270 machine type, suggesting that the behavior of the belt shift is consistently modified across different machine types under certain conditions. It's crucial to ensure that these changes are consistent and correctly implemented across all relevant parts of the firmware.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of belt shift behavior across different machine types. # Test: Search for conditional logic handling different machine types in relation to belt shifts. Expect: Consistent handling of belt shifts for KH270 and other types. rg --type cpp -A 5 $'Machine_t::Kh270'Length of output: 5256
Script:
#!/bin/bash # Description: Search for instances where BeltShift is used in conjunction with Machine_t::Kh270 to verify consistency. # Test: Search for BeltShift usage in relation to Machine_t::Kh270. rg --type cpp -A 5 'BeltShift' | rg 'Machine_t::Kh270'Length of output: 430
src/ayab/knitter.cpp (1)
Line range hint
390-422: Refactor suggestion forcalculatePixelAndSolenoid()method.The changes in this method streamline the calculation of
m_pixelToSetandm_solenoidToSetbased on the carriage direction and belt shift state. However, the logic can be further simplified or clarified:
- Simplification of Belt Shift Logic: The toggling of
beltShiftbased on the carriage type (Lace) could be encapsulated into a helper function to improve readability and reusability.- Magic Numbers and Documentation: The use of magic numbers directly in the code (e.g.,
HALF_SOLENOIDS_NUM) should be documented or defined as constants with descriptive names to improve code maintainability.- Conditional Logic for Solenoid Setting: The setting of
m_solenoidToSetbased onbeltShiftis clear, but the addition of3for theKh270machine type appears arbitrary without context. This should either be explained or refactored into a more descriptive mechanism.Consider applying the following refactor to encapsulate the belt shift logic:
bool Knitter::shouldInvertBeltShift(Carriage_t carriage) { // Inverts belt shift for Lace carriage as per the 930 service manual return Carriage_t::Lace == carriage; } // Usage in calculatePixelAndSolenoid() beltShift = shouldInvertBeltShift(m_carriage) ? !beltShift : beltShift;Additionally, define magic numbers as constants and add comments for unusual operations like the addition for
Kh270.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/ayab/encoders.cpp (6 hunks)
- src/ayab/knitter.cpp (3 hunks)
- test/mocks/beeper_mock.cpp (1 hunks)
- test/mocks/beeper_mock.h (1 hunks)
- test/test_encoders.cpp (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/ayab/encoders.cpp
Additional comments not posted (4)
test/mocks/beeper_mock.h (1)
38-38: Approved: Addition of theerrormethod inBeeperMock.The addition of the
errormethod is correctly implemented using theMOCK_METHOD0macro. This method will enhance the testing capabilities by allowing the simulation of error states, which aligns with the objectives of improving error handling in the system.test/mocks/beeper_mock.cpp (1)
66-69: New methodBeeper::error()correctly implemented.The addition of the
error()method to theBeeperclass is correctly implemented and follows the established pattern of usingassertto check for null pointers before proceeding with method calls ongBeeperMock. This ensures that the method behaves as expected without causing null pointer dereferences.test/test_encoders.cpp (1)
96-96: Updated Assertion for Belt ShiftThe change in the expected value for
getBeltShift()fromBeltShift::RegulartoBeltShift::Shiftedin the test casetest_encA_rising_in_front_notKH270reflects the updated logic in the firmware that handles belt shifts differently. This change should be verified to ensure it aligns with the intended behavior changes in the firmware, especially since it affects how the encoders interpret the position and movement of the carriages.src/ayab/knitter.cpp (1)
271-272: Clarification and error handling inknit()method.The addition of a call to
GlobalBeeper::error()in the event of an error during pixel and solenoid calculation is a good practice as it enhances error feedback to the user. This change aligns with the PR's objective to improve error signaling mechanisms.
|
Can I get a review? |
#179
Things in this PR:
Things that still need to be fixed (and will be before I request review):
After this PR, alignment issues will only ever show up at multiples of 8. I am so excited for this.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes