Wait for both selectors to clear the limit for end of row detection #200
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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-end-of-line
branch
from
8f2b5bd to
ce4b11b
Compare
jonathanperret
changed the title
jonathanperret
force-pushed
the
fix-end-of-line
branch
from
ce4b11b to
69da90e
Compare
We now maintain a "current line direction" that allows us to know whether the carriage is on the expected side of the working area for the line to be considered complete. Then we get rid of m_workedOnLine that is no longer useful. We also use the current line direction when determining which needle to program, making this more reliable than depending on the instantaneous carriage direction. Had to fix a few tests that used a mix of Knit and Garter carriage.
jonathanperret
force-pushed
the
fix-end-of-line
branch
from
69da90e to
11ad974
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #197, see there for a detailed description of the problem this PR is trying to address.
Proposed solution
As described in #197, the problem is likely caused by the firmware using the instantaneous carriage direction as given by the belt encoder to determine which of the active carriage's two "needle selectors" to consider when trying to determine whether the current row of knitting is complete.
With this PR, we instead maintain a "current line direction" variable (
m_currentLineDirection) that stores the expected direction of carriage travel for the current line, regardless of its immediate physical movement. This variable is initialized when knitting starts based on which Hall sensor was last passed, and flipped every time we detect a line is complete, as we request the next line from the computer.To detect that a line is complete, instead of using a flag (
m_workedOnLine) that alternates betweentrueandfalse, we use the current line direction to know whether to expect the carriage on the left or right end of the working area. This more securely guarantees that we don't trigger multiple end-of-row events while staying on the same end of the bed.The precise determination of the row being done is also improved. Not only do we check for the active needle selector (e.g. the one on the left side of the K carriage when moving right, or the one on the right side of the garter carriage also when moving right) to be past the last needle in work, we also check for the inactive needle selector to be past the last working needle.
Why is this important?
Let's look at a (schematized) K carriage moving right, hence with its active needle selector on the left side:
Once the active needle selector gets past the last needle in work, we're ready to turn around and start the new row (with the active and inactive selectors switching roles):
So far, so good. But what about a garter carriage in the same situation?
After a while, the active needle selector will have worked on the last active needle:
But if we were to turn the carriage around at this point, the newly-active selector would have missed a number of needles from the right end of the working area!
Therefore, it is imperative to wait for the inactive selector to pass the last needle, so that when it becomes active it can get started on what is now the first needle of the next row:
Waiting at both needle selectors to pass the last needle is a solution that works for both types of carriage, therefore this is what is implemented in this PR.