From 3f0c9d0a3297eaf0bcc19d0ef999e4853863c1ba Mon Sep 17 00:00:00 2001 From: Jonathan Perret Date: Mon, 30 Sep 2024 13:48:29 +0200 Subject: [PATCH] Fix clunking sound from solenoids 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-part 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 any way. Fixes #15. --- .gitmodules | 5 +--- lib/Adafruit_MCP23008 | 1 - src/ayab/solenoids.cpp | 55 ++++++++++++++++++++++++++++++----------- src/ayab/solenoids.h | 6 ++--- static_analysis.sh | 2 +- test/CMakeLists.txt | 9 ------- test/test_solenoids.cpp | 2 +- 7 files changed, 45 insertions(+), 35 deletions(-) delete mode 160000 lib/Adafruit_MCP23008 diff --git a/.gitmodules b/.gitmodules index 879382468..c20ca5710 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,6 +1,3 @@ [submodule "libraries/PacketSerial"] path = lib/PacketSerial - url = https://github.com/bakercp/PacketSerial.git -[submodule "libraries/Adafruit_MCP23008"] - url = https://github.com/adafruit/Adafruit-MCP23008-library.git - path = lib/Adafruit_MCP23008 + url = https://github.com/bakercp/PacketSerial.git \ No newline at end of file diff --git a/lib/Adafruit_MCP23008 b/lib/Adafruit_MCP23008 deleted file mode 160000 index 05fc9b8f6..000000000 --- a/lib/Adafruit_MCP23008 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 05fc9b8f68dd7e9f391aa80d4de1f0e5d396cbcb diff --git a/src/ayab/solenoids.cpp b/src/ayab/solenoids.cpp index b74c8124c..d1cb809ff 100644 --- a/src/ayab/solenoids.cpp +++ b/src/ayab/solenoids.cpp @@ -1,4 +1,4 @@ -/*! + /*! * \file solenoids.cpp * \brief Class containing methods that control the needles * via solenoids connected to IO expanders on the device. @@ -25,18 +25,29 @@ #include "solenoids.h" +// Minimal required constants from the MCP23008 datasheet: +// - fixed I2C address bits +#define MCP23008_ADDRESS 0x20 +// - used register addresses +#define MCP23008_IODIR 0x00 +#define MCP23008_GPIO 0x09 + /*! * \brief Initialize I2C connection for solenoids. */ void Solenoids::init() { - mcp_0.begin(I2Caddr_sol1_8); - mcp_1.begin(I2Caddr_sol9_16); + Wire.begin(); - for (uint8_t i = 0; i < SOLENOID_BUFFER_SIZE / 2; i++) { - mcp_0.pinMode(i, OUTPUT); - mcp_1.pinMode(i, OUTPUT); - } - solenoidState = 0x0000U; + // Default to all solenoids ON + // This mimicks the behavior of Brother electronics and is done + // to avoid a "clunking" sound that occurs if solenoids are turned + // OFF after having been ON while the carriage was moving. + solenoidState = 0xffffU; + writeGPIO(solenoidState); + + // Configure all MCP23008 pins as outputs + writeRegister(I2Caddr_sol1_8, MCP23008_IODIR, 0x00); + writeRegister(I2Caddr_sol9_16, MCP23008_IODIR, 0x00); } /*! @@ -59,7 +70,7 @@ void Solenoids::setSolenoid(uint8_t solenoid, bool state) { } if (oldState != solenoidState) { #ifndef AYAB_TESTS - write(solenoidState); + writeGPIO(solenoidState); #endif } } @@ -74,7 +85,7 @@ void Solenoids::setSolenoids(uint16_t state) { if (state != solenoidState) { solenoidState = state; #ifndef AYAB_TESTS - write(state); + writeGPIO(state); #endif } } @@ -93,9 +104,23 @@ void Solenoids::setSolenoids(uint16_t state) { * one bit per solenoid. */ // GCOVR_EXCL_START -void Solenoids::write(uint16_t newState) { - (void)newState; - mcp_0.writeGPIO(lowByte(newState)); - mcp_1.writeGPIO(highByte(newState)); +void Solenoids::writeGPIO(uint16_t newState) { + writeRegister(I2Caddr_sol1_8, MCP23008_GPIO, lowByte(newState)); + writeRegister(I2Caddr_sol9_16, MCP23008_GPIO, highByte(newState)); +} + +/*! + * Write to an MCP23008 register via I2C + * + * \param i2caddr Address of the I/O expander, only the 3 lowest bits are + * used, mapping to the A0..A2 address pins on MCP23008 + * \param reg Register address (see MCP23008 datasheet) + * \param value Value to set the register to + */ +void Solenoids::writeRegister(uint8_t i2caddr, uint8_t reg, uint8_t value) { + Wire.beginTransmission(MCP23008_ADDRESS | (i2caddr & 7)); + Wire.write(reg); + Wire.write(value); + Wire.endTransmission(); } -// GCOVR_EXCL_STOP +// GCOVR_EXCL_STOP \ No newline at end of file diff --git a/src/ayab/solenoids.h b/src/ayab/solenoids.h index f5c895806..47c62acd8 100644 --- a/src/ayab/solenoids.h +++ b/src/ayab/solenoids.h @@ -27,7 +27,6 @@ #include "board.h" #include "encoders.h" #include -#include #include // Different machines have a different number of solenoids. @@ -82,10 +81,9 @@ class Solenoids : public SolenoidsInterface { private: uint16_t solenoidState = 0x0000U; - void write(uint16_t state); + void writeGPIO(uint16_t state); - Adafruit_MCP23008 mcp_0 = Adafruit_MCP23008(); - Adafruit_MCP23008 mcp_1 = Adafruit_MCP23008(); + void writeRegister(uint8_t i2caddr, uint8_t reg, uint8_t value); }; #endif // SOLENOIDS_H_ diff --git a/static_analysis.sh b/static_analysis.sh index d1cfd1e91..34d8e1831 100755 --- a/static_analysis.sh +++ b/static_analysis.sh @@ -1,2 +1,2 @@ #!/bin/bash -clang-tidy src/ayab/$1.cpp --fix -- -isystem /usr/share/arduino/hardware/arduino/cores/arduino/ -isystem /usr/lib/avr/include/ -isystem /usr/share/arduino/hardware/arduino/variants/standard -isystem libraries/Adafruit_MCP23008/ -isystem /usr/share/arduino/libraries/Wire/ -isystem libraries/PacketSerial/src/ -DCLANG_TIDY "${@:2}" +clang-tidy src/ayab/$1.cpp --fix -- -isystem /usr/share/arduino/hardware/arduino/cores/arduino/ -isystem /usr/lib/avr/include/ -isystem /usr/share/arduino/hardware/arduino/variants/standard -isystem /usr/share/arduino/libraries/Wire/ -isystem libraries/PacketSerial/src/ -DCLANG_TIDY "${@:2}" diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 2fde0172e..081b33aa0 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -31,9 +31,6 @@ set(COMMON_INCLUDES ${SOURCE_DIRECTORY} ${LIBRARY_DIRECTORY}/PacketSerial/src ) -set(EXTERNAL_LIB_INCLUDES - ${LIBRARY_DIRECTORY}/Adafruit_MCP23008 - ) set(COMMON_SOURCES ${PROJECT_SOURCE_DIR}/test_boards.cpp @@ -85,16 +82,10 @@ set(COMMON_LINKER_FLAGS ${CMAKE_THREAD_LIBS_INIT} -lgcov ) -set(HARD_I2C_LIB - ${LIBRARY_DIRECTORY}/Adafruit_MCP23008/Adafruit_MCP23008.cpp - ) function(add_board board) set(processor # uno __AVR_ATmega168__ ) - set(I2C_LIB - ${HARD_I2C_LIB} - ) add_executable(${PROJECT_NAME}_${board} ${COMMON_SOURCES} # External libraries diff --git a/test/test_solenoids.cpp b/test/test_solenoids.cpp index 8dad99645..a13bbc6a0 100644 --- a/test/test_solenoids.cpp +++ b/test/test_solenoids.cpp @@ -50,7 +50,7 @@ TEST_F(SolenoidsTest, test_construct) { TEST_F(SolenoidsTest, test_init) { solenoids->init(); - ASSERT_TRUE(solenoids->solenoidState == 0U); + ASSERT_TRUE(solenoids->solenoidState == 0xffffU); } TEST_F(SolenoidsTest, test_setSolenoid1) {