[BUG] Mightyboard2560 RevE DIGIPOT pins are incorrectly defined

[idonasch]

Bug Description

Based on Marlin 2.0.7.2. Extruders are weak an can't push filament through. Problem is caused by failing to set the digipot values for the stepper drivers due to the fact that the SCL pin definition is wrong. Pin J5 assigned to IO-pin 83, not 76 according to the .platformio/packages/framework-arduino-avr-megacore/variants/100-pin-arduino-mega/pins_arduino. The same applies for DIGIPOTS_I2C_SDA_E1 which should be 84 (J6). also the MightyboardRevE does not supply a pull-up resistor and the SCL pin must be configure to use the CPU internal. this can be accomplished by the following redefinition of DIGIPOTS_I2C_SCL to #define DIGIPOTS_I2C_SCL 83,true // J5, pullup With these changes the extruders work properly.

Configuration Files

Required: Include a ZIP file containing Configuration.h and Configuration_adv.h.

If you've made any other modifications describe them in detail here.

Steps to Reproduce

start with a configuration based off ["examples/MBot/Cube/*.h], make super DIGIPOT_MCP4018 is defined.

1. platformio run -e MightyBoard2560 --target=upload
2. power cycle machine, if you flashed over a previous sailfish firmware w/o powercycling, the digipots are correctly configured and will mask the issue!

3. Try to extrude a few millimeters Expected behavior: a quiet extruder and sufficient extrusion of filament.
   Actual behavior: you will hear a clicking noise from the extruder and almost no filament will be extruded.

   Additional Information

   
   diff --git a/Marlin/src/pins/mega/pins_MIGHTYBOARD_REVE.h b/Marlin/src/pins/mega/pins_MIGHTYBOARD_REVE.h
   index 963911ec5d..1cd749908d 100644
   --- a/Marlin/src/pins/mega/pins_MIGHTYBOARD_REVE.h
   +++ b/Marlin/src/pins/mega/pins_MIGHTYBOARD_REVE.h
   @@ -108,12 +108,12 @@
   // Set from 0 - 127 with stop bit.
   // (Ex. 3F << 1 | 1)
   //
   -#define DIGIPOTS_I2C_SCL                      76  // J5
   +#define DIGIPOTS_I2C_SCL                      83,true  // J5, pullup
   #define DIGIPOTS_I2C_SDA_X                    57  // F3
   #define DIGIPOTS_I2C_SDA_Y                    61  // F7
   #define DIGIPOTS_I2C_SDA_Z                    65  // K3
   #define DIGIPOTS_I2C_SDA_E0                   27  // A5
   -#define DIGIPOTS_I2C_SDA_E1                   77  // J6
   +#define DIGIPOTS_I2C_SDA_E1                   84  // J6
   
   #ifndef DIGIPOT_I2C_ADDRESS_A
   #define DIGIPOT_I2C_ADDRESS_A             0x2F  // unshifted slave address (5E <- 2F << 1)

Adding the ',pullup' to the pin definition  is a kludge because I didn't want to touch digipot_mcp4018.cpp 

Remark:
I upgraded a 1280 board with a 2560 CPU and installed Optiboot. The platformio.ini section for the MightyBoard2560 diffs are:

'[env:MightyBoard2560] -platform = atmelavr -extends = common_avr8 -board = ATmega2560 -upload_protocol = wiring -upload_speed = 57600 -board_upload.maximum_size = 253952 +platform = atmelavr +extends = common_avr8 +board = ATmega2560 +upload_speed = 115200 +board_upload.maximum_size = 261120'

* Provide pictures or links to videos that clearly demonstrate the issue.
[MBot_Configuration.zip](https://github.com/MarlinFirmware/Marlin/files/5684891/MBot_Configuration.zip)

* See [Contributing to Marlin](https://github.com/MarlinFirmware/Marlin/blob/2.0.x/.github/contributing.md) for additional guidelines.

[boelle]

i'm a bit in doubt here, is the above also a fix to the issue?

[idonasch]

Yes, it includes the fix. to verify that the fix is correct, compare pins with the platformio...variants/100-pin-arduino-mega/pins_arduino.h file. In the original Marlin code the pin designation (in the comment) is correct, just the numbers don't match the pins_arduino.h defines.

The fix for the missing pullup also works, but maybe splitting that into different defines is a better approach, allowing for the pin definition somewhere else to be used as intended.

The 2 lines is question from pins_MIGHTYBOARD_REVE.h should read:

#define DIGIPOTS_I2C_SCL 83,true // J5, pullup
#define DIGIPOTS_I2C_SDA_E1 84 // J6

[grauerfuchs]

This looks like an erroneous definition issue specific to MegaCore. The extended 2560 pins definition/variant does not have this issue. I've been working (offline) with testing a switch from the MegaCore libs to the mega2560ext variant libs instead, and it's looking very promising. On all three of my official FFCP w. 2560 machines, the ext libs seem to be more reliable and cleaner. Rather than changing the pins definition directly, we might want to simply throw the switch and do this in platformio.ini instead:

Line 488:
-board                     = ATmega2560
+board                     = megaatmega2560
+board_build.variant       = megaextendedpins
+extra_scripts             = ${common.extra_scripts}
+                            pre:buildroot/share/PlatformIO/scripts/copy_marlin_variant_to_framework.py

I also recommend not altering the bootloader details since the default onboard bootloader (from CTC and FlashForge, at least, SailFish compatile) matches the values currently listed in the platformio.ini.

[ellensp]

@grauerfuchs changing MightyBoard to megaextendedpins was one of the intentions when I added it, but I never got hold of a mightyboard to verify it works to complete the move.. If you can verify it works fully we can push that change through.

[idonasch]

I tested the approach with the board_build.variant = megaextendedpins and it assigned the correct pins but still left the issue with the missing pullup unaddressed.

@ellensp To fix that it is still required to add true as the 3rd parameter to all SlowSoftI2CMaster constructors to set _pullup to true. e,g, SlowSoftI2CMaster(DIGIPOTS_I2C_SDA_X, DIGIPOTS_I2C_SCL, true) in digipot_mcp4018.cpp

@grauerfuchs referring just to env:mega2560ext this in platformio worked for me and preserved backward compatibility.

[env:MightyBoard2560]
extends       = env:mega2560ext

At least on my Mightyboard2560 the issue could be resolved with both of these changes.

[grauerfuchs]

@idonasch Excellent. Please keep in mind that the project is going for general compatibility, so we have to keep the environment configured for what most users would encounter. Your customization of the bootloader clearly works, and it does have benefit both in upload speed and size, but many users won't have the ISP or won't want to get involved with such advanced (and breaking) changes. Might I ask, what was the main goal in switching out the bootloader? Was it for performance reasons, program size, upload speed, or something else?

@ellensp It looks to me like we are go to fix the MightyBoard2560 env by updating the board definition to megaatmega2560 and setting the env as an extension to mega2560ext. At that point we can remove the protocol and size parameters, since they were only needed to override settings in MegaCore and they already match the megaatmega2560 defaults. We do, however, need to keep the speed parameter since it's non-standard.

The next step is addressing the MCP4018 pull-ups. I'm going to go back over the documentation and changes to see if I can find where this issue came in. It's possible that some change in the library somewhere may be involved. It's also possible the change in bootloader might be involved. All I can say on this so far is that I did not have any issues with the digipots when I completed my testing for the support of raw wiper values, and I haven't experienced issues since. However, I also haven't done any dual extrusion prints since that change was tested and there have been many changes to the code and libraries following that commit.

[idonasch]

Regarding the bootloader: I started out with the mega1280 cpu and had trouble getting my configuration squeezed into the limited space. Optiboot came to the rescue and saved me a few kB but only briefly. Now I have swapped the CPU and started configuring more features in. Reflashing between Sailfish and Marlin (w/o power cycling) mask the issue for quite some time... Therefore optiboot is not required any more!

Regaring Pull-ups: I2C protocol requires open-drain with (usually 4k7) pull-ups. Mightyboard does not have them (I checked the eagle files). The SlowSoftI2CMaster library uses a workaround by using the CPU internal open-drain pin configuration with internal ~40k pullups. Drawback of this is that there is always a brief (and by atmel design unavoidable) time where the pins are floating. I guess that's why the library author decided to make this an option but turning it off be default. If an other board is equipped with these (actually required) external pull-up, the patch wont hurt et all and therefore I recommend to make it the default. (the SlowSoftI2CMaster::setLow/High() functions are showing how the "pseudo-open-drain" writes are done)

I hope this sheds some light on how I ran into this issue and I'm glad you guys jumped right on it!, Thanks a lot!

[grauerfuchs]

Understood. That's one of the main reasons I upgraded one of my other machines from the (admittedly nice) 1284p to a 32-bit board. There simply wasn't enough progmem to fit the features I wanted, even with all the optimizations turned on.

The main reason I wanted to go back through everything is because you're correct; I2C requires the pull-ups. However, I know for a fact that the digipots were programmed successfully while I was running tests (a year ago or so?) to implement raw wiper access that would allow me to run custom TMC2100 and TMC2208 driver boards with full vref control in software. Therefore, I'm a little concerned as to when and where the change occurred that seems to have broken this control for you. It's possible the default bootloader may initialize the pins to the proper state and the library doesn't override it. If so, your Optiboot configuration probably does not preset the pins correctly and therefore the issue becomes apparent. It's also possible that a former version of the SlowSoftI2CMaster library or a previous alternative may have by default initialized with pull-ups whereas the current code does not. If so, then that's a break/bug that should be patched as the default operation. Otherwise, there may be a reason that internal pull-ups are not enabled by default and we do not see it in our scope of fixing the Mightyboard. If that's the case, then enabling the pull-ups needs to be made an option which is utilized only where appropriate.

It's amazing how something that looks so easy on the surface can quickly snowball into something more complicated, especially since we have to make sure it works and compiles (whether used or not) across so many platforms.

[grauerfuchs]

Confirmed, the loss of internal pull-ups appears to be a bug caused when the project moved from the purpose-made custom implementation of SlowSoftI2CMaster (https://github.com/stawel/SlowSoftI2CMaster ; still listed in the MCP4018 code) to the common implementation provided via PlatformIO (https://github.com/felias-fogg/SlowSoftI2CMaster). This means it's a likely candidate for making the pull-ups a default. My only concern on this would be whether or not another supported environment has a board using MCP4018 where the internal pull-ups are either at an incompatible voltage or are not available due to system limitations. I think it unlikely, but it's still something to consider.

[idonasch]

@grauerfuchs OK, this is for clarification only and can be considered off-topic and is in no means a critique of yours or any anyone else work on this project: All your concerns are mostly caused by electrical engineers not making their homework. 1) Hardware has to ensure that voltage levels can not exceed the devices spec, e.g if a 5V MPU connects to 3.3V I2C device there shall be a voltage converter in between (e.g. 2 FETs) otherwise software must ensure not to set these pins high in push-pull mode! Dangerous! Luckily MAX4018 are 5V tolerant. (https://www.microchip.com/wwwproducts/en/MCP4018) 2) I2C spec requires open drain protocol since both master and slave are allowed to pull SCL low, although most devices are fast enough or the MPU's are too slow anyways. 3) Both SoftwareI2CMaster libraries you mentioned ignore the fact that any I2C device can slow down the transfer speed by holding SCL low by itself. This would require the SoftwareI2CMaster library to monitor SCL until it's actually high even when itself has already releases the pin. Not complying to 3) violates 2)!!!

As software engineers we need at least to know these things in order to decide if we can ignore them in a particular project. Sorry, but I had to get this off my chest. In my career I always enjoyed working with HW engineers before they confronted me with a finished design (and vv. sigh)

Again, thank you for finding the root cause and I now leave it to you to find and promote the proper solution. I consider this issue thoroughly understood and solved and will stay out of any further discussions unless anybody wants me explicitly to pitch in.

Merry Chistmas Ingo

[grauerfuchs]

I agree. If everything had been handled properly in hardware, we wouldn't be in this mess in the first place. Unfortunately, people rarely seem to do the sensible thing when there's a cheaper, faster option available.

platformio.ini env and MCP4018 I2C issues have been addressed in PR #20493.

[grauerfuchs]

@idonasch The fixes have been merged, it's compiling without error, and the digipot setting appears to be working on my machines. Please pull the latest bugfix-2.0.x and give it a try.

[idonasch]

@grauerfuchs digipots now set properly on CTC replicator1 with MightyBoard_REVE. bugfix-2.0.x from Fri Dec 18 00:15:25 2020

[thisiskeithb]

Closing as resolved.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.