Fix clunking sound from solenoids (#15) #203
Conversation
WalkthroughThe pull request introduces several changes primarily focused on the removal of the Adafruit MCP23008 library and its associated functionality. The Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
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: 4
🧹 Outside diff range and nitpick comments (2)
test/test_solenoids.cpp (1)
53-53: Approved: Initial solenoid state change aligns with PR objectives.This change correctly updates the test to expect all solenoids to be active (0xffffU) upon initialization, which aligns with the PR objective of preventing the clunking sound by keeping solenoids engaged by default. This new behavior mimics the Brother electronics as described in the PR summary.
Consider adding the following additional tests to ensure comprehensive coverage of the new behavior:
- Test turning off specific solenoids and verify they remain off.
- Test turning all solenoids off and ensure they don't produce the clunking sound (you may need to mock the hardware interaction for this).
- Test the behavior when restarting a pattern to ensure solenoids remain engaged.
These additional tests will help validate that the new implementation correctly addresses the issue in various scenarios.
src/ayab/solenoids.cpp (1)
28-34: Useconstexprinstead of#definefor constantsUsing
constexprvariables provides type safety and better scoping compared to preprocessor#definedirectives.Consider changing the macros to
constexprconstants:-#define MCP23008_ADDRESS 0x20 -#define MCP23008_IODIR 0x00 -#define MCP23008_GPIO 0x09 +constexpr uint8_t MCP23008_ADDRESS = 0x20; +constexpr uint8_t MCP23008_IODIR = 0x00; +constexpr uint8_t MCP23008_GPIO = 0x09;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- .gitmodules (1 hunks)
- lib/Adafruit_MCP23008 (0 hunks)
- src/ayab/solenoids.cpp (5 hunks)
- src/ayab/solenoids.h (1 hunks)
- static_analysis.sh (1 hunks)
- test/CMakeLists.txt (0 hunks)
- test/test_solenoids.cpp (1 hunks)
💤 Files with no reviewable changes (2)
- lib/Adafruit_MCP23008
- test/CMakeLists.txt
🔇 Additional comments (2)
.gitmodules (1)
1-3: LGTM! Removal of Adafruit_MCP23008 submodule aligns with PR objectives.The removal of the Adafruit_MCP23008 submodule from the
.gitmodulesfile is in line with the PR objectives. This change:
- Addresses the clunking sound issue by removing the library that was forcing all outputs to turn off.
- Simplifies the codebase by eliminating a third-party dependency.
- Allows for direct control of the MCP23008 expanders using the Arduino Wire library.
The remaining PacketSerial submodule is unchanged, which is appropriate if it's still needed for the project.
Let's verify that the Adafruit_MCP23008 library has been completely removed from the project:
src/ayab/solenoids.cpp (1)
107-110: LGTM: Correct implementation ofwriteGPIOmethodThe
writeGPIOfunction correctly writes the lower and higher bytes of the solenoid state to the appropriate I2C addresses, ensuring proper control over the solenoids.
jonathanperret
force-pushed
the
fix-clunking-sound
branch
from
1135e26 to
78468f9
Compare
The "clunking" sound that is occasionnally heard when using AYAB is caused by all solenoid armatures being released simultaneously when all solenoids are turned off. This does not happen with Brother electronics because they default to keeping solenoids on. This commit changes AYAB's behavior to match that behavior of the Brother electronics. Note that this required abandoning the third-party library used to drive the I/O expanders (Adafruit_MCP23008) since part of its non-skippable initialization code was forcing all outputs to off. It turns out the amount of code needed to directly drive the MCP23008 expanders using the Arduino Wire library is only a dozen lines of code anyway. Fixes AllYarnsAreBeautiful#15.
jonathanperret
force-pushed
the
fix-clunking-sound
branch
from
78468f9 to
90a296c
Compare
|
So much clutter from coderabbit here... is there some action item from me? Or why am I marked for review? |
Sorry, I thought you might be interested in looking at this, but no action is required. Agree that coderabbit is a bit much. |
Problem
Fixes #15.
Proposed solution
The "clunking" sound that is occasionnally heard when using AYAB is caused by all solenoid armatures being released simultaneously when all solenoids are turned off.
This does not happen with Brother electronics because they default to keeping solenoids on.
This PR changes AYAB's behavior to match that behavior of the Brother electronics.
Note that this required abandoning the third-party library used to drive the I/O expanders (
Adafruit_MCP23008) sincepart of its non-skippable initialization code was forcing all outputs to off. It turns out the amount of code needed to directly drive the MCP23008 expanders using the Arduino Wire library is only a dozen lines of code anyway.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
0xffffU.Chores