Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i2c_software: Implementation of software i2c #6141

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

bigtreetech
Copy link
Contributor

@bigtreetech bigtreetech commented Mar 27, 2023

Follow up #6130
Software I2C tested on STM32H7 with both BME280 and sh1106, The actual test results are normal.

The following are some results captured by the logic analyzer.

  • 100KHz i2c read
    a3d2b996336782eda56e5e4d4d3e12cb

  • 100KHz i2c write
    e9e9c51ef7d78c9b4b3c7f0e34123bb4

  • 400KHz i2c read
    dd6d0a3ea9c1503cbac3354e6f4c6617

  • 400KHz i2c write
    ff0a17ebe4b980061b658321ed68b08a

Note

  • The maximum rate in the .c source file is limited to 1MHz, and the actual rate captured by the logic analyzer is between 800KHz-1MHz Speed fixed to 100KHz
  • Tested on Mega2560, it can control i2c devices normally, but even without the i2c_delay function, the occupancy rate of the mcu is still 100%, so it is not recommended to use software i2c on MCU like Mega2560

@thijstriemstra
Copy link
Contributor

Tested on Mega2560, it can control i2c devices normally, but even without the i2c_delay function, the occupancy rate of the mcu is still 100%, so it is not recommended to use software i2c on MCU like Mega2560

Any other test results? e.g an Octopus board.

@KevinOConnor KevinOConnor self-assigned this Apr 5, 2023
Copy link
Collaborator

@KevinOConnor KevinOConnor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. As high-level feedback, the functionality seems useful. I have some comments on the implementation - see below.

Can you provide additional information on how this will be used? Is there a device that needs this support and requires high speed I2C access?

-Kevin

Comment on lines 81 to 86
gpio_out_write(is->sda, b & 0x80);
b <<= 1;
i2c_delay(is->ticks);
gpio_out_write(is->scl, 1);
i2c_delay(is->ticks);
gpio_out_write(is->scl, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To communicate over an I2C bus, the transmitter should transition between GND and high-Z states. This code is transitioning between GND and HIGH states. If the transmitter transitions to a HIGH state while the receiver is in a GND state it would result in a "short" on the wires that would consume power, generate heat, and possibly destabilize the board voltage.

I have written code to communicate with I2C devices using just GND/HIGH transitions (eg, klippy/extras/mcp4018.py). This change, though, may confuse users into thinking Klipper supports I2C on any device. I'm not sure that is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Theoretically this is a problem.
It would be perfect if there were a universal API to set GPIO as Open-Drain output, but some MCU (such as RP2040) do not seem to have the Open-Drain feature

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the code transmit bits by switching between gpio_in_reset(is->sda_in, 1) and gpio_out_reset(is->sda_out, 0)?

I suspect the transfer rate would be slower, but it would hopefully not cause electrical problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the transfer rate would be slower. Mainly affecting MCU with lower main frequencies such as STM32G0B1 64MHz.
The measured results are as follows

1 gpio_out_write gpio_in/out_reset
STM32G0B1 70KHz 30-40KHz
RP2040 84KHz 70KHz

For MCUs with high main frequencies like STM32H7, The impact is minimal.

src/i2c_software.c Outdated Show resolved Hide resolved
src/i2c_software.c Outdated Show resolved Hide resolved
@bigtreetech bigtreetech force-pushed the software_i2c branch 4 times, most recently from 8f1e835 to 55b5e92 Compare April 17, 2023 07:50
@bigtreetech
Copy link
Contributor Author

Tested on Mega2560, it can control i2c devices normally, but even without the i2c_delay function, the occupancy rate of the mcu is still 100%, so it is not recommended to use software i2c on MCU like Mega2560

Any other test results? e.g an Octopus board.

It can work normally in Octopus F4/H7 versions

@KevinOConnor
Copy link
Collaborator

Thanks. I guess my main question is - which users are expected to utilize this and what hardware is it expected that they will use it with? Is this just general functionality that some users may find useful or is there a specific hardware setup that requires this functionality?

In general I prefer to only add code when we have a general idea of who will be using it and what they use it for. (That way, I can get a better feel for the maintenance costs of the extra code relative to the functionality obtained by that new code.)

Cheers,
-Kevin

@bigtreetech
Copy link
Contributor Author

Thanks. I guess my main question is - which users are expected to utilize this and what hardware is it expected that they will use it with? Is this just general functionality that some users may find useful or is there a specific hardware setup that requires this functionality?

In general I prefer to only add code when we have a general idea of who will be using it and what they use it for. (That way, I can get a better feel for the maintenance costs of the extra code relative to the functionality obtained by that new code.)

Cheers, -Kevin

This requirement is raised by some of our customers who want to connect BME280 temperature and humidity sensors to our motherboard (such as Octopus max ez). There is no hardware i2c port reserved on the motherboard, but there are many GPIO

@KevinOConnor
Copy link
Collaborator

Okay, thanks. In general, it looks fine to me. I'll look to commit early next week.

-Kevin

@KevinOConnor KevinOConnor merged commit 645a1b8 into Klipper3d:master Jun 8, 2023
@KevinOConnor
Copy link
Collaborator

Thanks.

-Kevin

@bigtreetech bigtreetech deleted the software_i2c branch June 8, 2023 01:53
@JamesH1978
Copy link
Collaborator

This looks to break smaller STM32 builds like the 042 with an error of

Linking out/klipper.elf /usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: out/klipper.elf section.text' will not fit in region rom' /usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: region rom' overflowed by 904 bytes
collect2: error: ld returned 1 exit status`

Seems to push it over the flash size

Thanks
James

@KevinOConnor @bigtreetech

@JamesH1978
Copy link
Collaborator

seems changing line 10 of Kconfig from select HAVE_GPIO_I2C if !MACH_STM32F031 to select HAVE_GPIO_I2C if !MACH_STM32F031 && !MACH_STM32F042 fixes it at least for the 042, i havent tested other STM32F0XX mcu's

@Lefuneste83
Copy link

seems changing line 10 of Kconfig from select HAVE_GPIO_I2C if !MACH_STM32F031 to select HAVE_GPIO_I2C if !MACH_STM32F031 && !MACH_STM32F042 fixes it at least for the 042, i havent tested other STM32F0XX mcu's

This seems to allow compilation of FW for STM32F042. I have one in a Voron 0 display board. But the commit 645a1b seems to break i2c communication nonetheless. I have made several attempts, including a complete reinstall of klipper, but they all failed and I had to rollback to previous commit, which works without a problem, both in compiling FW and communication with the display board.

Protocol error
Traceback (most recent call last):
  File "/home/pi/klipper/klippy/klippy.py", line 180, in _connect
    cb()
  File "/home/pi/klipper/klippy/mcu.py", line 753, in _connect
    self._send_config(None)
  File "/home/pi/klipper/klippy/mcu.py", line 683, in _send_config
    cb()
  File "/home/pi/klipper/klippy/extras/bus.py", line 177, in build_config
    self.i2c_write_cmd = self.mcu.lookup_command(
  File "/home/pi/klipper/klippy/mcu.py", line 873, in lookup_command
    return CommandWrapper(self._serial, msgformat, cq)
  File "/home/pi/klipper/klippy/mcu.py", line 84, in __init__
    self._cmd = msgparser.lookup_command(msgformat)
  File "/home/pi/klipper/klippy/msgproto.py", line 315, in lookup_command
    self._error("Unknown command: %s", msgname)
  File "/home/pi/klipper/klippy/msgproto.py", line 243, in _error
    raise error(self.warn_prefix + (msg % params))
msgproto.error: mcu 'display': Unknown command: i2c_write

@JamesH1978
Copy link
Collaborator

then my commit/solution may be just a sticking plaster for other problems caused by this commit

@KevinOConnor
Copy link
Collaborator

See #6244 for code to optionally disable software i2c (and other features) on chips with small flash sizes.

-Kevin

@JamesH1978
Copy link
Collaborator

This also seems to break some rp2040 boards, at least a couple of SB2040 mellow owners have tmc uart issues on this commit, that i have seen on the discord

Thanks
James

@Sineos
Copy link
Collaborator

Sineos commented Jun 12, 2023

Lesorin pushed a commit to Lesorin/klipper that referenced this pull request Jul 13, 2023
Signed-off-by: Alan.Ma from BigTreeTech <tech@biqu3d.com>
Lesorin pushed a commit to Lesorin/klipper that referenced this pull request Jul 13, 2023
Signed-off-by: Alan.Ma from BigTreeTech <tech@biqu3d.com>
rogerlz referenced this pull request in DangerKlippers/danger-klipper Dec 19, 2023
Signed-off-by: Alan.Ma from BigTreeTech <tech@biqu3d.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants