ESP32 IDF4 Build - I2C.Setup function results in Invalid Arguments on clock speed

[SimonGAndrews]

An error occurs using espruino in the ESP32-C3 IDF 4 build. when setting up the i2C.

In more detail: When building with the command line:
BOARD=ESP32C3_IDF4 DEBUG=1 make flash

The error occurs when executing the espruino command:
I2C1.setup({scl:D9,sda:D8});

The error text is :

i2c: i2c_param_config(684): i2c clock choice is invalid, please check flag and frequency 
Uncaught Error: jshI2CSetup: Invalid arguments at line 1 col 27 
I2C1.setup({scl:D9,sda:D8}); 

[SimonGAndrews]

This link seems to identify the problem.
And tracing the error message back into the code shows that the ESP IDF i2c driver (home/Espruino/esp-idf-4/esp-idf/components/driver/i2c.c) function i2c_get_clk_src() , inside i2c_param_config() is expecting to be passed . clk_flags in the config structure from espruino jshardwareI2c.c

i2c_get_clk_src() proides clock selection functionality in the IDF V4.4 for the specific device..

[SimonGAndrews]

The fix is to pass . clk_flags in the config structure from espruino jshardwareI2c.c function jshI2CSetup().
This needs to consider that Espruino Javascript function i2c.setup() can an pass an optional required bitrate as in
I2C1.setup({sda:D8,scl:D9,bitrate:1000000});
The IDF uses a combination of .master.clk_speed and .clk_flags to select one of the available clocks (and hence the speed) for the specific target device. According to the docs above , setting .clk_flags to Zero should allow the master.clk_speed parameter to solely control the selected clock but this does not seem to work as intended and always selects the fasted available clock for the device. So a bit of a hac is required to set the clk_flag also depending upon the required bitrate.
Another consideration is that Espruino (in a debug build) prints to console, the speed to which the i2c is initialised, but there is not a function in the IDF to actually recall the speed that was setup, so the printed speed needs to be what the driver was given and assuming that it is using that speed. So ideally Espruino need to use the clock select logic in the IDF also to get this correct.
In summary the fix needs to use the requested bitrate to get a clock and its speed then report that on initialisation. The method employed was to hardcode, round up the requested bitrate to the next available clock for the specific target device up to the max clock speed available .

This change effects the ESP32-C3 and -s3 builds and leaves the ESP32 build as is. Both the C3 and S3 have the same available clocks/speeds.
The fix also sets the device dependent SCl and SDA pins.

Note this will all need to change anyway in IDF V5 as a new driver is used and needs modifying when /if new ESP32 devices are included in the build until IDF V5 is deployed.

EDIT - On Testing the final change was not as discussed here - SEE BELOW

[SimonGAndrews]

testing is WIP

[gfwilliams]

Thanks! This all looks good - if you could get your changes as a PR (or just a patch I can apply) that'd be great.

On the I2C pin front, on STM32 where we have a similar situation of hard-coded pins, we have a checkPinsForDevice function which calls jshFindPinForFunction(device, functions[i]); which works out what pin to used based on the auto-generated jspininfo.c file: https://github.com/espruino/Espruino/blob/master/targets/stm32/jshardware.c#L2532-L2535

So I think you could do:

if (!jshIsPinValid(info->pinSCL)) info->pinSCL = jshFindPinForFunction(device, JSH_I2C_SCL);
if (!jshIsPinValid(info->pinSDA)) info->pinSDA = jshFindPinForFunction(device, JSH_I2C_SDA);

Which would work if the pininfo file was correct - I'm not sure it is right now, I think we only do UART: https://github.com/espruino/Espruino/blob/master/boards/ESP32.py#L187-L190

But it would be a nice one to fix, as it's also used for the nice graphics in https://www.espruino.com/ESP32#gpio-pins - for example the one for an STM32 board shows what peripherals are where: https://www.espruino.com/Pico#pinout

[SimonGAndrews]

Thanks,Working toward PR.
Using a fixed board file for the pins is much cleaner and thanks for the clues , should be able to sort that.

[gfwilliams]

Thanks! I've just pushed changes that add the relevant data for I2C1 - I wasn't too sure about UART/SPI/etc on other boards

[SimonGAndrews]

@gfwilliams reccomendations to use the jshFindPinForFunction() is now working , with his board definition file changes, in my test version with this code:

In addition Ive changed ESP32.Py to include pin definitions for I2C2 with :

Ive tested the setup in both the ESP32-C3 and ESP32 builds. These test builds are from the action workflows in my github clone of Espruino. The default Pins are now working on both for I2C1 and on the ESP32 for I2C2**. The debug logging you can see in the new I2Csetup function above results in the following test results:

1. ESP32> I2C1.setup(); -> I2C pins on device: I2C1, identified as sda: 22, scl: 21
2. ESP32> I2C2.setup(); -> I2C pins on device: I2C2, identified as sda: 17, scl: 16
3. ESP32> I2C1.setup({ scl : D4, sda: D5 }); -> I2C pins on device: I2C1, identified as sda: 5, scl: 4
4. ESP32> I2C2.setup({ scl : D4, sda: D5 }); -> I2C pins on device: I2C2, identified as sda: 5, scl: 4
5. ESP32> I2C1.setup({ scl : D22, sda: D21 }); -> I2C pins on device: I2C1, identified as sda: 21, scl: 22
6. ESP32-C3> I2C1.setup(); -> I2C pins on device: I2C1, identified as sda: 8, scl: 9
7. ESP32-C3> I2C2/setup(); -> Uncaught ReferenceError: "I2C2" is not defined
8. ESP32-C3> I2C1.setup({sda:4,scl:3}); -> I2C pins on device: I2C1, identified as sda: 4, scl: 3

(further test results and PR to follow)

[gfwilliams]

That looks excellent - thanks for your work on this!

[SimonGAndrews]

** regarding the default I2C2 pins on the ESP32. I have made sure that on both I2C1 and I2C2 the default pins have not changed from the previous Espruino builds to avoid any breaking changes. However:
for i2C1, the sdl and scl pin defaults on the current ESP32 build are actually switched around from the common ESP32 documentation. Espruino has assigned SDA as P22 and SCl as P21 , where has common documentation and the espressif Arduino Library has SCL as P22 and SDA as P21.
and for I2C2, the existing pins as SDA 17 and scl 16 seem not to be documented anywhere.

With this in mind I would suggest (And will change in my PR) adding this to the documentation for i2c.setup in jswrap-spi_i2c

[gfwilliams]

for i2C1, the sdl and scl pin defaults on the current ESP32 build are actually switched around

Have you been able to test? I would assume with ESP32 that what Espruino did was most likely wrong to begin with - so if you can change it to match what everyone else did I think it would be better to do that than to try and soldier on with nonstandard pins

[SimonGAndrews]

Hi , Yes I Did test, and rechecked today. Ive checked with an I2C desplay device including monitoring with a logic analyser.

Conclusion as before: On an ESP32 target pre any changes (Espruino 2V24 ) using default (ie no config object passed in I2C.setup() ) , for correct operation a device needs to be wired with SDA on P22 and SCL on 21. This is opposite to the documentation and pinout diagrams ive found.
So following your advice I will correct this in the ESP32.py file.

[gfwilliams]

Wow, ok - thanks! That's crazy. I wonder what's up with that then...

[SimonGAndrews]

On final testing , I realised that setting the required clock speed was not just a matter of setlecting the correct clock via the . clk_flags (as discussed above). The IDF also uses the value passed in .master.clk_speed against the selected clock to set the actual speed.
During testing a logic analyser was used to check the actual speed the I2C was using.

Speed setting tests using both of the available clocks on the ESP32-C3 were measured using the analyser and it was concludeded to hardcode the selection of the 2MZ XTAL clock by setting the .clk_flags to Zero. This gave an accurate clock speed up to 400 Khz (or 400000 bitrate setting) . I was not successful in setting a clock much higher than this accurately. Maybe my measurment method, wiring pullups etc, or just the device cant do it.

So the final change in the jshI2CSetup Looked like this.

This leaves the ESP32 device I2C speed selection as was before the change.
Note there are further considerations in the clock selection eg the other clock (1MHz RTC clock ) is documented as being better for low power. But will leave to others to invistigate and possible add the clg_flags to the espruino i2Csetup function config parameters . In any event as above these changes will need further update for IDF V5.

[SimonGAndrews]

Finally all builds were sucessfull in the github build workflow of my Espruino clone. So going to raise my first PR with these changes. :)

[gfwilliams]

Just merged - thanks for all your work on this! It's great that you were able to check the speeds with the logic analyser too!