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

XMC2Go: Wire.begin() sends I2C start condition #234

Closed
9Volts9er opened this issue Mar 27, 2023 · 18 comments
Closed

XMC2Go: Wire.begin() sends I2C start condition #234

9Volts9er opened this issue Mar 27, 2023 · 18 comments

Comments

@9Volts9er
Copy link
Contributor

Hi,

I am using the XMC2Go and the Release 2.1.0 of XMC-for-Arduino in order to communicate with an I2C device.
There I have the issue that my device always ignored the first read/write command. I simply do not get an ACK. However, when I send the command a second time, it works. The issue only happens once after startup, so the first I2C communication after startup was ignored. Later commands were not affected.
So I had a closer look to this with a logic analyzer.

There I found that the Wire.begin() happens to pull down the SDA and SCL pins:
632_466_1

According to the I2C specification this can be interpreted as an I2C start condition, because the SDA is pulled LOW while SCL is idle (HIGH).

I traced this down to the following two code lines inside the Wire.begin():
Link to code

XMC_GPIO_Init((XMC_GPIO_PORT_t*)XMC_I2C_config->sda.port, (uint8_t)XMC_I2C_config->sda.pin, &(XMC_I2C_config->sda_config));
XMC_GPIO_Init((XMC_GPIO_PORT_t*)XMC_I2C_config->scl.port, (uint8_t)XMC_I2C_config->scl.pin, &(XMC_I2C_config->scl_config));

Those code lines seem to be responsible for pulling down at first SDA and then SCL.

A workaround for me was to exchange those lines with each other. Now the SCL gets pulled LOW, and then afterwards SDA, so this is not recognized as start condition anymore, and so the communication worked without any problems.

XMC_GPIO_Init((XMC_GPIO_PORT_t*)XMC_I2C_config->scl.port, (uint8_t)XMC_I2C_config->scl.pin, &(XMC_I2C_config->scl_config));
XMC_GPIO_Init((XMC_GPIO_PORT_t*)XMC_I2C_config->sda.port, (uint8_t)XMC_I2C_config->sda.pin, &(XMC_I2C_config->sda_config));

687_477_1

However, this is just a workaround, and maybe just for my specific case, it's not a really good solution. I'm still not sure, why the lines get pulled down in the first place...
Maybe someone with a better understanding of the code could help here?

Thanks a lot.

@techpaul
Copy link
Contributor

As this is FIRST use of a pin after CPU RESET, I suspect that what is needed is not ordering of lines, but a write to the output register to set both bits High, BEFORE configuring pin as output open drain. It looks like the internal GPIO register is cleared to 0 on power up/reset, so when the configuration is made the first thing that happens is the EXISTING state of the internal output register (0) is connected to the pin.

This sort of mod would be required to BOTH types of beigin as Master and Slave.

Also the pins are for some strange reinitialised to same settings in end() which is odd, as they should already be initialised before getting to end(). Suspect some tidy up there as well.

A quick look, something like the following NOT TESTED but suggestion for you to try -

XMC_GPIO_SetOutputLevel( , , XMC_GPIO_OUTPUT_LEVEL_HIGH );

For each pin probably with references to the references

(uint8_t)XMC_I2C_config->scl.port
(uint8_t)XMC_I2C_config->scl.pin,

Giving two extra line BEFORE the init of

XMC_GPIO_SetOutputLevel( (uint8_t)XMC_I2C_config->scl.port, (uint8_t)XMC_I2C_config->scl.pin,, XMC_GPIO_OUTPUT_LEVEL_HIGH );

XMC_GPIO_SetOutputLevel( (uint8_t)XMC_I2C_config->sda.port, (uint8_t)XMC_I2C_config->sda.pin, XMC_GPIO_OUTPUT_LEVEL_HIGH );

In both begin() and begin(address) functions BEFORE the XMC_GPIO_Init(...) calls

@9Volts9er
Copy link
Contributor Author

Wow, thank you for the super quick reply :)

I tried the suggested solution, but it seems that it does not work.
This is what I inserted in both Wire.begin() functions:

XMC_GPIO_SetOutputLevel( (XMC_GPIO_PORT_t*)XMC_I2C_config->scl.port, (uint8_t)XMC_I2C_config->scl.pin, XMC_GPIO_OUTPUT_LEVEL_HIGH );
XMC_GPIO_SetOutputLevel( (XMC_GPIO_PORT_t*)XMC_I2C_config->sda.port, (uint8_t)XMC_I2C_config->sda.pin, XMC_GPIO_OUTPUT_LEVEL_HIGH );

directly before those two lines:

XMC_GPIO_Init((XMC_GPIO_PORT_t*)XMC_I2C_config->sda.port, (uint8_t)XMC_I2C_config->sda.pin, &(XMC_I2C_config->sda_config));    
XMC_GPIO_Init((XMC_GPIO_PORT_t*)XMC_I2C_config->scl.port, (uint8_t)XMC_I2C_config->scl.pin, &(XMC_I2C_config->scl_config));

But unfortunately I still see the lines going LOW

@jaenrig-ifx
Copy link
Member

The initial value of the i2c lines is set to HIGH for all boards and configurations:
https://github.com/Infineon/XMC-for-Arduino/blob/master/libraries/Wire/src/utility/xmc_i2c_conf.c

So, maybe the reordering of the functions is enough for not falling into that start condition. But I would expect that such transient of sda and scl going low should not happen any time given that both are pulled-up to VDD, and the init is setting the HIGH as well.

@techpaul
Copy link
Contributor

That is what should be happening, but I suspect the issue is order of configuring pins with I2C_init and I delved into gpio_init that appears to set level AFTER setting pin configuration, this may well be within th depths of XMCLIB

In most cases this won't be noticed, it is noticeable in I2C due to state transitions by order of execution.

In reality it should not be possible.

Setting mode then setting default level is a problem on many platforms where people do not set level before configuring as output. In most cases the users or equipemnt does not notice the glitch when just driving an LED or similar.

When configuring pins for interrupts it is often advisable to ensure pin is suitable state to avoid strange glitches.

I will see if I can delve further into the code, between other things.

@techpaul
Copy link
Contributor

@9Volts9er

From you analayser traces it lokks like the time between first edge going low and edge going high ias around 12 micro seconds, is this always a fixed time?

Is the difference between the two edges going low always the same time?

This suggests code execution timing, along with fact they both go high almost the same time when the I2C actually takes over

@9Volts9er
Copy link
Contributor Author

Yes, those times are always the same.
I just measured the time between the two edges going LOW 4 times in a row, and it was always the same:
960_701_1
Also for the time from the first falling edge to the rising edges stays the same over multiple startups...

@techpaul
Copy link
Contributor

I suspect the order of calls in Wire.cpp compared to what XMCLIB functions are doing especially with HWSEL (who has control of pins), as the XMC_GPIO_Init calls are in the middle of I2C config HWSEL may be being changed all over the place, and then set correct at last call in function XMC_I2C_CH_Start

XMC_GPIO_Init does many things
Switch to input mode
Disable HWSEL
Set hysteresis
Check if analog pin special mode needed
Set level
Set mode

Two tests I would suggest

  1. MOVE the two XMC_GPIO_Init calls to just before XMC_I2C_START_CH, should see the timings reduce
  2. MOVE the two XMC_GPIO_Init calls to just before XMC_I2C_CH_Init, so all I2C manipulation is done after GPIO manipulation

I suspect there is an interaction somewhere causing problems with many things possibly the HWSEL and changing things one pin ata time whilst the I2C hardware deasl with two pins simultaneously

@9Volts9er
Copy link
Contributor Author

I rearranged the code as suggested.

  1. As expected, the time reduces when the XMC_GPIO_Init functions are moved just before XMC_I2C_START_CH:
    1055_460_1

  2. When moved before XMC_I2C_CH_Init, the time increases significantly (the time between both falling edges stays the same with 4.28µs):
    1045_450_1

Additionally, I had a look into XMC_I2C_CH_Start(XMC_I2C_config->channel); because I wanted to see, what command pulls the lines HIGH again. It actually seems that this function does nothing more than configuring the USIC channel as I2C:

__STATIC_INLINE void XMC_I2C_CH_Start(XMC_USIC_CH_t *const channel)
{
  XMC_USIC_CH_SetMode(channel, XMC_USIC_CH_OPERATING_MODE_I2C);
}

The XMC_USIC_CH_SetMode is directly modifying the CCR register of the channel:

__STATIC_INLINE void XMC_USIC_CH_SetMode(XMC_USIC_CH_t *const channel, const XMC_USIC_CH_OPERATING_MODE_t mode)
{
  channel->CCR = (uint32_t)(channel->CCR & (~(USIC_CH_CCR_MODE_Msk))) | (uint32_t)mode;
}

So that means, as soon as the USIC channel is configured to act as I²C, the I²C lines are released. Logical next step for me was trying to move the XMC_GPIO_Init() to the end of the function, so after XMC_I2C_CH_Start().

Result: It works, the lines are not pulled LOW anymore.

With this modification I understand that the USIC Channel is completely up and running as I²C before the GPIOs are switched to HW-controlled.
Do you think changing this could have any negative effects, e.g. with the Interrupts?

@techpaul
Copy link
Contributor

Thanks for doing those tests, as suspected the issue is how XMC_GPIO_Init is functioning, which is different order to the version of XMC1100 processor manual I have checked through which mentions orders to avoid glitches. See Reference manual for XMC1100AB, TPage 18-10 onwards Section 18.7 Initialization and System Dependencies, order is supposedly disable HWSEL before changing to input (clearing all bits for that pins mode). Manual states when setting utput to set HWSEL as one of the LAST things, there is no resetting of HWSEL before exit, even to what it was before.. So either the HWSEL disable is not working correctly or when USIC mode is set to I2C it over rides any form of HWSEL setting, the other cause is the source control in this function.

There is no mention in these sections about source control which may be a major cause of issues as in init xmc_i2c_conf.c sets the source to XMC_INPUT_F and XMC_INPUT_E, where as in end they are set to XMC_INPUT_A. So the source is set to USIC pins, BEFORE the mode is configured, so if the USIC output is zero that is passed across to output regardless of output register (fine for SPI not for I2C).

Doing what you are doing is LESS lkely to affect interrupts as these are defined by the I2C setup of the USIC and removing glitches lessens chnces of false interrupts.

When under Hardware Control configurations of the port settings registers are NOT altered as the hardware control is after the registers, which is why I think the GPIO init in end is unnecessary.

At the moment I fail to see that HWSEL is actually doing anything for I2C configuration, as it is not enabled by the time XMC_I2C_CH_Start, the hardware appears to takeover regardless. As your tests show, it does not matter where XMC_GPIO_Init is the I2C can work (with or without glitches.

I would suggest, trying commenting out XMC_GPIO_Init calls in begin() of Wire.cpp, and test operations for start and communications. and checking that open drain gets configured by removing a pullup..

To ME it looks like at least with USIC configuration of pins it over rides probably ALL configuratuons of pin regardless, or at worst only the GPIO pins source needs changing AFTER starting I2C mode. Especially as you have things failing on the first transaction only.

@9Volts9er
Copy link
Contributor Author

Sorry, I tried, but I'm not sure if I understood every part of your answer correctly.
However, here is my answer to what I think you meant. Please do not hesitate to correct me if I understood something wrong.

(I labeled by answer with numbers in order to structure it a little)

  1. As you suggested, I tried to comment out the XMC_GPIO_Init() calls. But then nothing works anymore. There is no I²C communication on the lines visible. The GPIOs are open drain (Output goes LOW when removing the Pullup), but I guess, this is because they are initially in this state after MCU reset and then they simply are never configured.
    And to my naïve mind this observation somehow makes sense, because the XMC_I2C_CH_Start only writes to the channel->CCR register, which does not configure any GPIOs. So when I commented out the XMC_GPIO_Init() calls, the HWSEL-bits for the SDA- and SCL-pins are never set to HW-controlled.
    That's why I think the XMC_GPIO_Init() are necessary in the Wire.begin().

  2. After the XMC_I2C_CH_Init() there are the lines that set the input source for the USIC channels to XMC_INPUT_F and XMC_INPUT_E:

XMC_USIC_CH_SetInputSource(XMC_I2C_config->channel, XMC_USIC_CH_INPUT_DX0, XMC_I2C_config->input_source_dx0);
XMC_USIC_CH_SetInputSource(XMC_I2C_config->channel, XMC_USIC_CH_INPUT_DX1, XMC_I2C_config->input_source_dx1);

But when I looked into those functions I only see modifications to the USIC channel registers, not yet the GPIO/Port registers. So the pins are not affected as long as they are not set to HW-controlled via the HWSEL-bits, aren't they?

  1. You also mentioned the Reference Manual section 18.7, page 18-10. There it says:

When a port pin is configured as output for an on-chip peripheral, it is important that the
peripheral is configured before the port switches the control to the peripheral in order to
avoid spikes on the output.

Aren't those "spikes" exactly what I observed?
So actually moving the XMC_GPIO_Init() calls to the end of the Wire.begin() function would be the correct solution according to this note in the Reference Manual, isn't it?

  1. The question is how to proceed here? For my case the solution with moving the XMC_GPIO_Init() calls to the very end of the Wire.begin() function works perfectly fine. Do you think, it would be okay to create a Pull Request with my suggested solution?
    Then we could test it further and if there are no other issues visible we might take over the changes in the main branch...

@techpaul
Copy link
Contributor

The first problem is that HWSEL is being used funny, and I think that your fix that fixes a part of the problem is that XMC_I2C_CH_Start has grabbed some form of H/W control BEFORE XMC_GPIO_Init can do anything, and XMC_GPIO_Init "supposedly" ONLY DISABLES hardware control (in wrong order) and never reenables it.

In Section 18.3 it states
Depending on the operating mode, the peripheral can take control of various functions:
• Pin direction, input or output, e.g. for bi-directional signals
• Driver type, open-drain or push-pull
• Pull devices under peripheral control or under standard control via Pn_IOCR

But nowhere could I find what aspects of hardware control it actually does for any pin or peripheral.
So we do not know if redefinition of pin later will happen or not under hardware control.

Your tests
1/ Shows some apects of XMC_GPIO_Init are needed but that has mitakes accoring to manual for transitions of port pin and recoonfigurations is

  • DIsable HWSEL
  • Set Input mode
    It is accually done in reverse order

When setting a port you should ENABLE HWSEL as last action, this is NOT done, could be acheived by reading before disbling and rewriting saved value as last thin in XMC_GPIO_Init. This means reconfigurations later could be scuppered.

2/ We do not know from manual if ALL actions occur at XMC_I2C_CH_Start or part way through.

3/ Yes you are getting spikes, yes this is part of the solution. It has highlighted to me other potential issues like do we actually need to do XMC_GPIO_Init in Wire.end(), does this issue also occur in SPI, or other functions to do with timers, can the port be reconfigured after an end() correctly, also need to check versions of XMCLIB and see if better XMC_GPIO_Init exists in newer versions.

4/ What to do see if you can do a Pull Request for develop (V2.1x) and develop-1.x (V1.7x) a both versions exist and both have the SAME problem.

I as a fellow contributor thank you for your willingness to do tests, and having a hardware setup to test this on,

@techpaul
Copy link
Contributor

I have looked through code this problem affects
SPI
HardwareSerial

XMC_GPIO_Init issues affect all boards

HardwareSerial on end does NOT do some things like set the USIC channel to idle, and they HALF did the fix of moving one of the XMC_GPIO_init before XMC_UART_CH_Start not after it

I will see what has to be done, when I get a few minutes.

@techpaul
Copy link
Contributor

techpaul commented Apr 6, 2023

Now an important related update has been merged into develop I will try to find tiime to fix file for I2C,SPI, HardwareSerial and I2S. Later a fix for XMCLIB

@9Volts9er
Copy link
Contributor Author

Thanks for keeping this topic alive and sorry for my delayed answer. I had a lot of other stuff going on the last few weeks and couldn't find the time to further work on this topic :/
But I guess I will be able to setup the pull requests according to your suggestions somewhen next week, if this would be of any help for you. Of course if you say you prefer to implement this yourself in order to have the correct solution right away, I will not touch anything ;)

@techpaul
Copy link
Contributor

Like you I have many things on including doing other Arduino based boards projects involving team of people and multiple customers in a long chain, taking up most of my time.

If you can get I2C sorted for develop (V2.x branch) and develop-1.x (V1.7x +) branch it would help, and I can look t others later.

Main things re

  • in begin move the GPI_Init to after XMC_I2C_CH_Start
  • Remove in end()
    XMC_GPIO_CONFIG_t default_output_port_config structure
    XMC_GPIO_Init line

Basically in end we are duplicating what ws done in begin and if someone has externally changed the port pin config before calling end, either way will cause issues and it is unnecessary code.

I will look at other iUSIC nterfaces and updating XMCLib version then change needed in there when I get some time, a few pull requests for that.

9Volts9er added a commit to 9Volts9er/XMC-for-Arduino that referenced this issue Apr 20, 2023
Put the GPIO_Init() after CH_Start for SPI, I2C and HardwareSerial in order to avoid false pulses as described in issue Infineon#234
9Volts9er added a commit to 9Volts9er/XMC-for-Arduino that referenced this issue Apr 20, 2023
Put the GPIO_Init() after CH_Start for SPI, I2C and HardwareSerial in order to avoid false pulses as described in issue Infineon#234
@9Volts9er
Copy link
Contributor Author

I've implemented the changes as you suggested and created the Pull Requests #237 and #238.
Thank you very much for your help with this.

I will close the issue as soon as the pull requests are merged.

@techpaul
Copy link
Contributor

Had a look and only minor thing missed was moving the sclk call to GPIO.init in HW_SPI.cpp

@boramonideep
Copy link
Contributor

Thanks @9Volts9er for highlighting this issue and also providing the fix. Thanks @techpaul for the support.

jaenrig-ifx pushed a commit that referenced this issue Jun 20, 2023
Put the GPIO_Init() after CH_Start for SPI, I2C and HardwareSerial in order to avoid false pulses as described in issue #234
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

No branches or pull requests

4 participants