Encoders: only look at leftmost edge of left sensor signal #218
Conversation
The code in encA_rising used to reset the carriage position to "zero" for every needle where the sensor was triggered. This caused the zero position to be taken as the rightmost position where a magnet was detected (a magnet is usually detected for two to three needle widths). In turn this caused solenoids to sometimes be activated too late to correctly select needles. By checking for the signal going from "no magnet detected" to "any magnet detected", the "zero" position will be set only at the leftmost edge of the magnet detection area. This should shift everything left by one or two needles, leaving more time for the solenoids to be activated.
WalkthroughThe changes modify the carriage detection mechanism in the Changes
Assessment against linked issues
Note: The changes improve code modularity and detection logic, but a direct resolution of the lace carriage issue cannot be conclusively determined from this code review alone. Further testing would be required to validate the fix. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (8)src/ayab/encoders.h (1)
The new member variable and method declaration support the PR objective of improving left sensor timing logic. The Also applies to: 191-191 src/ayab/encoders.cpp (3)
The method cleanly encapsulates the logic for determining carriage type based on hall sensor readings, making the code more maintainable.
Correctly initializes the previous carriage state during setup.
The new logic only triggers on transitions from mid-level to magnet-present state, which should address the timing issues mentioned in PR #40. However, we should verify this improvement. test/test_encoders.cpp (4)
The
Correctly sets up the initial state with mid-level sensor reading.
The test case thoroughly verifies the new carriage detection logic, including:
Good use of 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 (
|
|
Thanks for the review!
Good ideas, but unrelated to the issue, and I'm trying to minimize changes in this patch, since this code is so fragile currently and I'm not satisfied with the current test suite. My plan is to clean all this up once we get a full automated test suite running.
Yes, this is the plan, but since the right sensor is essentially not supported currently, I thought it would be simpler to only touch the left sensor code for now and get feedback on whether this fixes issue #40. I'm working on the right sensor code anyway in #205, I'll integrate this logic change if it works. |
|
@jonathanperret / @Adrienne200 indicated that this solves #40. |
Problem
Hopefully fixes #40.
Theory of the issue
My current theory for why needles are sometimes wrongly selected (going to position
Dinstead ofB) is that the associated solenoid is activated too late in the process, causing the solenoid's armature to no longer be in contact with the solenoid core anymore by the time it gets powered, resulting in the selection lever not being prevented from moving the selector plate and selecting the needle toD.In principle, this could be solved by simply increasing the "timing advance" (
START_OFFSETinayab-firmware) for activating solenoids when the carriage moves, i.e. instead of selecting for needle0when the carriage center is at needle4as we do currently, we could select for needle0as soon as the carriage center is over needle3(or even earlier).But in the particular case of the lace carriage, this would interfere with the initial carriage detection. Because of the existence of the garter carriage, when we detect a lace carriage's magnet at the left Hall sensor, we wait for a few more needles before declaring to the desktop app that a lace carriage is present and requesting the first line's data. If an opposite polarity magnet is detected in that interval, we instead report a garter carriage. Also, once we have determined the carriage type and requested the line data, even though it normally arrives with milliseconds of the request, the firmware currently needs to wait for the next needle before effectively applying the data to the solenoids.
This adds up to a situation where we can't start selecting solenoids from the pattern until the lace carriage's center is over needle 4 (L97). If we are using a pattern that goes up to needle 0 (L100), the solenoid for that needle will be activated right after we receive the line data. But if we changed the timing advance to select for needle
0when the carriage is over needle3instead, then that needle would never get selected properly, since by the time we have the pattern data we have already reached needle4.A round of testing by @Adrienne200 with a machine affected by #40 showed however that the alternative firmware https://github.com/jpcornil-git/ayabAsync-firmware was not exhibiting the issue.
After comparing the code of the two firmwares, and measuring their timing behavior, it appeared that they were both using the same timing advance (selecting for needle
0when the center is over needle4). So, something else had to be different.Looking into the left Hall sensor logic as suggested by @jpcornil-git proved promising: it turns out
ayabAsync-firmwaredoes something different thanayab-firmwarethere. Whereayab-firmwarecurrently resets the carriage position to zero as long as a magnet is detected in front of the sensor (effectively declaring that needle0is at the rightmost position where the magnet can be detected),ayabAsync-firmwareinstead places needle0at the position where the sensor signal reaches an extremum (minimum for the lace carriage).Since we have observed that the area where the sensor was triggered by a magnet could reach up to three needles in width, this could create a difference of one to two needles in the timing of solenoid activation between the firmwares. This could be enough to make the difference between issue #40 manifesting or not.
Proposed solution
In this PR, we change the logic for the left Hall sensor to reset the carriage position to "zero" (actually
END_LEFT_PLUS_OFFSETbut that corresponds to needle0) only when the sensor signal is seen going from a mid-level value to a "magnet present" value, while the carriage is moving left. In effect this sets the "zero" position at the leftmost edge of the magnet detection area. This should shift everything left by one or two needles, leaving more time for the solenoids to be activated.Summary by CodeRabbit
New Features
Bug Fixes
Tests