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

[OLED] Don't change shared bus speed #3208

Merged

Conversation

tonhuisman
Copy link
Contributor

Removed Wire.clockSpeed() call as a library shouldn't change a shared resource.

Resolves #3207

@TD-er
Copy link
Member

TD-er commented Aug 12, 2020

Maybe also add a warning to MLX90614 plugin to set it to 100 kHz I2C?

The nicest way would be to have it as boolean in the device definition (begin of a plugin), and then the web interface automatically show the warning when it is applicable.

@tonhuisman
Copy link
Contributor Author

Maybe also add a warning to MLX90614 plugin to set it to 100 kHz I2C?

The nicest way would be to have it as boolean in the device definition (begin of a plugin), and then the web interface automatically show the warning when it is applicable.

Well, that would imply that ESPEasy has in-depth knowledge about the technical specs/limitations of the plugins it supports, and one would expect that warning for other plugins as well.
And what if there is an updated version of the hardware that does support higher clock speeds? Or even a mixture of both old and new versions of that hardware?

A more feasible option could be to have a configurable I2C clock speed for each I2C device, where 0=default to the global setting. (As one of the suggestions in this forum post)

@TD-er
Copy link
Member

TD-er commented Aug 12, 2020

I don't know if variable clock speed will work on the I2C bus.
For truly high speed I2C, you add an extra flag in the communication and also the frequency is quite a bit higher so any filter in the slower devices won't even see the high clock.

@TD-er
Copy link
Member

TD-er commented Aug 12, 2020

Maybe we can also have a new device type...
We ow have types like I2C, UART, etc. which also help populate their settings page and view info on the device overview tab.
Maybe there is room for an I2C_LOW_SPEED one, which also allows to change the I2C speed to a lower value for those devices that need it and set it back to the set value in the global settings after the calls.

But like I said, I don't know if a truly low speed device (100 kHz) may cause issues when it may not recognize other commands on the bus for other devices.

@thomastech
Copy link
Contributor

But like I said, I don't know if a truly low speed device (100 kHz) may cause issues when it may not recognize other commands on the bus for other devices.

That is a concern. A low speed device might cause bus problems if the i2c clocking is increased for other devices. To avoid that possibility it seems that the slowest device would globally determine the max bus speed that should be used.

  • Thomas

@TD-er
Copy link
Member

TD-er commented Aug 12, 2020

Yep, I think we should first obey the KISS principle and only give a warning on the problematic plugin and just rely on the global I2C clock settings.

@TD-er TD-er merged commit 64205bc into letscontrolit:mega Aug 14, 2020
@tonhuisman tonhuisman deleted the bugfix/OLed_dont_change_wirespeed branch August 15, 2020 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove I2C frequency change from Framed OLED library
3 participants