[BFW-5420] Feature/i2c m260 m261

[rbuehlma]

This adds support for the m260 m261 commands and makes it possible to control an I2C device attached to the I2C socket.

Related to https://github.com/prusa3d/Prusa-Firmware-Buddy/issues/3711

Since I'm not used to developing c++ and even less for a firmware, please let me know if something needs to be changed to make it possible to include this change.

Please not that this change requires EXPERIMENTAL_I2CBUS to be enabled to be effective and therefore requires a custom firmware build.

[rbuehlma]

I did rebase the code to the current release v6.0.0

[danopernis]

Hi @rbuehlma thanks for the rebase and sorry about the force-push and the wait.

Looking at the code I don't think this would work. Buddy platform is providing Arduino-compatible API for Marlin to use, See src/common/hwio* files and digitalWrite() function and those i2c pins are not exposed. But even if they were, bit banging the i2c would not work because the bits are in i2c mode controlled by the stm32f4 processor and they should be controled via STM HAL Interface (HAL_I2C_*() functions). Special care must be taken to not cause any deadlock and not trigger the main task watchdog. What is more, you do not want to enable just any gcode to communicate via i2c as there are other peripherals attached there and a malicious attacker could craft a special gcode to for example destroy EEPROM constents.

While I like the general idea, it needs to be reimplemented entirely to conform with the buddy architecture and safety checks must be in place.

P.S. Holly/Jenkins build system is broken right now, do not worry about it.

[3d-gussner]

Please don't forget to update https://reprap.org/wiki/G-code#M260:_i2c_or_Modbus_Send_Data and https://reprap.org/wiki/G-code#M261:_i2c_or_Modbus_Request_Data

[rbuehlma]

Thanks for the feedback. I'm not really used to develop firmwares and am therefore happy for any input :)

Technically, this change is working (I'm using it to control a laser header through an I2C port expander but this required additional changes (see below)). The calls to the i2c bus are with this patch done through HAL_I2C_Master_Transmit. Did I still miss something there?

I agree that malicious gcode has an additional attack vector by enabling this feature but taking gcode from an untrusted source has already been a risk before. By blacklisting already used I2C addresses, I believe it would be possible to fix this.

During my testings, I never had deadlocks caused by this code, but I guess the watchdog could be triggered if some device takes too long to respond (this never happened to me).

If you are interested in what else I did to get my laser header working correctly, this is the corresponding branch: https://github.com/rbuehlma/Prusa-Firmware-Buddy/tree/feature/laser Don't be afraid, I won't create a PR for this one as I had to dig too much into the movement processing to get the timing of the laser header to be in sync with the movement :) I'm sure there must be a much simpler way.

[danopernis]

Oh shoot you are right, I was somehow only looking into the first commit :facepalm:

This already looks much better :slightly_smiling_face: and I will open an internal ticket [BFW-5420] for us to see what we can do about addressing the security issues raised and getting this merged.

We will also need to solve which i2c HAL handle to use. There are several of them and each has different devices attached, differing between MINI, MK4 and XL depending on the particular electronics. For this, I will have to look into electronic schemas.

[rbuehlma]

Great, thanks. Just FYI: The first commit is simply a C&P from the current Marlin Repo to have their current I2C commands implementation. The second commit is what I did to get it running with the Buddy firmware.

[rbuehlma]

Thanks @michalrudolf

I have updated the PR according to your suggested changes.

[rbuehlma]

Hell indeed, I had the flag disabled while compiling this version. Now it works with the flag as well.

Would it be a good idea to enable the flag by default such that users of the official firmware can make use of this feature?

[michalrudolf]

Yes, you can uncomment it. Right now MINI, MK4, MK3.5 and XL can have it enabled.

[rbuehlma]

I2C is now enabled. I did additionally verify that it still works after these changes.

[michalrudolf]

Enable for XL, not for XL_Dwarf or XL_Dev_Kit (atleast for now).

[rbuehlma]

All done.

[michalrudolf]

Looks like our CI have a problem. There is a simple solution:

Remove this from Marlin.h

#if ENABLED(EXPERIMENTAL_I2CBUS)
  #include "feature/twibus.h"
#endif

and this from Marlin.cpp

#if ENABLED(EXPERIMENTAL_I2CBUS) && I2C_SLAVE_ADDRESS > 0

  void i2c_on_receive(int bytes) { // just echo all bytes received to serial
    i2c.receive(bytes);
  }

  void i2c_on_request() {          // just send dummy data for now
    i2c.reply("Hello World!\n");
  }

#endif

Could you please squash/rebase the fixup commits, so we have a few well defined commits in this PR?

[rbuehlma]

All done :)

[Fil4ment]

Please forgive me for being a noob at github, but I am, so I ask: How do I tell what release this commit has been/is going to be merged into, or when it's in the public release?

[danopernis]

@Fil4ment it is good that you ask, it is not so obvious and we are still trying to create a workflow that would work best for us.

We are generally trying to track public pull requests in the release notes. Other than that, we cherry-pick bugfixes to patch versions. Features usually do not make it to patch versions, but eventually end up in minor/major versions. Regardless of the release they actually end up in, we always merge them to the master branch here.

Some day, we should write some nice documentation about that, along with some general info for contributors. It gets complicated because lot of development happens behind closed doors (for reasons we developers don't really have control over)