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

[BUG] ESP32-S3 Stuck in adc functions #295

Open
CommanderRedYT opened this issue Jul 23, 2023 · 17 comments · Fixed by #414
Open

[BUG] ESP32-S3 Stuck in adc functions #295

CommanderRedYT opened this issue Jul 23, 2023 · 17 comments · Fixed by #414

Comments

@CommanderRedYT
Copy link

Describe the bug
When trying for example the Inline current sensing test, the ESP32-S3 (esp32-s3-devkitc-1 v1.0), the esp32 is not printing anything. After some debugging, we discovered that it is stuck in adcRead (https://github.com/simplefoc/Arduino-FOC/blob/master/src/current_sense/hardware_specific/esp32/esp32s_adc_driver.cpp). We found that the while loop checking if the adc is ready is causing the problem (with our pin configuration, its the bottom one in line 241):

if(channel > 7){
while (GET_PERI_REG_MASK(SENS_SAR_MEAS2_CTRL2_REG, SENS_MEAS2_DONE_SAR) == 0); //wait for conversion
value = GET_PERI_REG_BITS2(SENS_SAR_MEAS2_CTRL2_REG, SENS_MEAS2_DATA_SAR, SENS_MEAS2_DATA_SAR_S);
} else {
while (GET_PERI_REG_MASK(SENS_SAR_MEAS1_CTRL2_REG, SENS_MEAS1_DONE_SAR) == 0); //wait for conversion
value = GET_PERI_REG_BITS2(SENS_SAR_MEAS1_CTRL2_REG, SENS_MEAS1_DATA_SAR, SENS_MEAS1_DATA_SAR_S);
}

When adding this line of code, I can see "reg: 80160000" inside the serial monitor:

Serial.printf("reg %x\n", READ_PERI_REG(SENS_SAR_MEAS1_CTRL2_REG));

Describe the hardware setup
For us it is very important to know what is the hardware setup you're using in order to be able to help more directly

  • Which motor: None (devboard only)
  • Which driver: None
  • Which microcontroller: Espressif ESP32-S3-DevKitC-1
  • Which position sensor: None
  • Current sensing used? Yes

IDE you are using

[env:esp32-s3-devkitc-1]
platform = espressif32
board = esp32-s3-devkitc-1
framework = arduino
lib_deps = askuric/Simple [email protected]
lib_archive = false
monitor_speed = 115200
upload_speed = 921600

We tried this with multiple esp32-s3 boards, and did not get it to work. When using this platformio configuration for normal esp32 (not S3!), it works however (same code):

[env:esp32dev]
platform = [email protected]
board = esp32dev
framework = arduino
lib_deps = askuric/Simple [email protected]
lib_archive = false
monitor_speed = 115200
upload_speed = 921600
@runger1101001
Copy link
Member

Have you tried with the dev branch code of SimpleFOC and the most recent version of the espressiv32 framework?

@runger1101001 runger1101001 removed their assignment Sep 2, 2023
@mailonghua
Copy link

Has this issue been resolved? I also encountered the same problem

@CommanderRedYT
Copy link
Author

@mailonghua We still have to test it, haven't had time yet. I will post updates when we find out more

@mailonghua
Copy link

@mailonghua We still have to test it, haven't had time yet. I will post updates when we find out more

I think I have located this problem. This is a bug. Each ADC of esp32s3 has 10 channels 0~9, and the SENS_SAR_MEAS1_CTRL2_REG register shows the status of ADC1, and SENS_SAR_MEAS2_CTRL2_REG shows the status of ADC2. But when I use GPIO9, currently The judgment detects BIT16 of SENS_SAR_MEAS2_CTRL2_REG, which belongs to ADC2. When I change if(channel > 7) to if(channel > 9), my current program is normal.
But I think this method may not be appropriate for ADC2

@mailonghua
Copy link

@mailonghua We still have to test it, haven't had time yet. I will post updates when we find out more

I think I have located this problem. This is a bug. Each ADC of esp32s3 has 10 channels 0~9, and the SENS_SAR_MEAS1_CTRL2_REG register shows the status of ADC1, and SENS_SAR_MEAS2_CTRL2_REG shows the status of ADC2. But when I use GPIO9, currently The judgment detects BIT16 of SENS_SAR_MEAS2_CTRL2_REG, which belongs to ADC2. When I change if(channel > 7) to if(channel > 9), my current program is normal. But I think this method may not be appropriate for ADC2

Sorry, I may have forgotten to note a setting. In addition to the above modifications, you also need to read it before currency.init when using the analogread() interface. The current sensor will be initialized normally. The specific reason has not been determined yet, but I guess it may be Related ADC enable issues

@CommanderRedYT
Copy link
Author

@mailonghua We still have to test it, haven't had time yet. I will post updates when we find out more

I think I have located this problem. This is a bug. Each ADC of esp32s3 has 10 channels 0~9, and the SENS_SAR_MEAS1_CTRL2_REG register shows the status of ADC1, and SENS_SAR_MEAS2_CTRL2_REG shows the status of ADC2. But when I use GPIO9, currently The judgment detects BIT16 of SENS_SAR_MEAS2_CTRL2_REG, which belongs to ADC2. When I change if(channel > 7) to if(channel > 9), my current program is normal. But I think this method may not be appropriate for ADC2

Sorry, I may have forgotten to note a setting. In addition to the above modifications, you also need to read it before currency.init when using the analogread() interface. The current sensor will be initialized normally. The specific reason has not been determined yet, but I guess it may be Related ADC enable issues

What exactly do you mean by currency.init? Also, isn't analogread Arduino.h?

@mailonghua
Copy link

@mailonghua We still have to test it, haven't had time yet. I will post updates when we find out more

I think I have located this problem. This is a bug. Each ADC of esp32s3 has 10 channels 0~9, and the SENS_SAR_MEAS1_CTRL2_REG register shows the status of ADC1, and SENS_SAR_MEAS2_CTRL2_REG shows the status of ADC2. But when I use GPIO9, currently The judgment detects BIT16 of SENS_SAR_MEAS2_CTRL2_REG, which belongs to ADC2. When I change if(channel > 7) to if(channel > 9), my current program is normal. But I think this method may not be appropriate for ADC2

Sorry, I may have forgotten to note a setting. In addition to the above modifications, you also need to read it before currency.init when using the analogread() interface. The current sensor will be initialized normally. The specific reason has not been determined yet, but I guess it may be Related ADC enable issues

What exactly do you mean by currency.init? Also, isn't analogread Arduino.h?

My phenomenon is that it is blocked during the initialization process of the InlineCurrentSense object, that is, current_sense.init();, which is ultimately blocked to the register location you mentioned above waiting for the ADC to complete.
I am currently using the PlatformIO platform. In addition to the modifications I mentioned above, I use the ardunio interface analogread() to read the two pins of the current detection before initializing the current sensor. Then the blockage of the current sensor initialization will no longer exist.

@CommanderRedYT
Copy link
Author

Just as FYI, we still did not have time to test it, but we might be able to do it until new year. (Pending)

@runger1101001
Copy link
Member

We have just merged this PR: #346 to the dev branch.

It is related to problems with ADC2. Perhaps it also solves your issue?

@CommanderRedYT
Copy link
Author

We have just merged this PR: #346 to the dev branch.

It is related to problems with ADC2. Perhaps it also solves your issue?

Is possible, yes. I will try it out in the next few days and then report here if everything works

@SwapnilPande
Copy link

We have just merged this PR: #346 to the dev branch.

It is related to problems with ADC2. Perhaps it also solves your issue?

I've been following this issue on the ESP32-S3 too and trying to track it down. I mirrored the changes from this PR into the ESP32s driver with no success.

Digging a little deeper, it seems that the driver is not actually fully configuring the ADC correctly. Unless I'm misunderstanding, it looks like the driver for the ESP32-S* microcontrollers comments out the configuration for the ADC to avoid compilation errors (contributed in #198). It looks like these functions need to updated to match the new registers.

The code seems to be getting stuck waiting for an ADC conversion here. This seems consistent with incomplete ADC configuration. I'm going to try to track down a fix for this over the next few weeks, but help would be greatly appreciated. I'm not very familiar with the S3!

@CommanderRedYT
Copy link
Author

@SwapnilPande I think #346 looks very promising. This is the exact place where we found out that is was looping. Sadly, we are still very busy. But i am very positive that this is fixed now. (Still lets keep this open until we checked it with our hardware)

@SwapnilPande
Copy link

@SwapnilPande I think #346 looks very promising. This is the exact place where we found out that is was looping. Sadly, we are still very busy. But i am very positive that this is fixed now. (Still lets keep this open until we checked it with our hardware)

I tried #346 on my hardware and I found that I was still getting stuck in initializing the inline current sensing. It would be interesting to see if your experience is different.

@mailonghua
Copy link

I encountered the same issue and also tried #346, but it didn't resolve the current problem. I am still waiting for the completion of ADC2. I am currently using the esp32-s3.

@mailonghua
Copy link

I discovered that in addition to resolving bug #346, if I use analogRead to read the sensor pin before sensor initialization, and then proceed with the initialization, blocking issues do not occur. I have dumped the following registers:

0x60008838: 0xfffff
0x60008824: 0x60050001
0x60008830: 0x80260000
0x6000883c: 0x0
0x60008820: 0x7000f3
0x60008818: 0x10001
0x60008840: 0x0

However, I have not yet identified the problem. Has anyone else encountered this issue?

askuric added a commit that referenced this issue Jun 21, 2024
@askuric
Copy link
Member

askuric commented Jun 21, 2024

Hey guys,

I've investigated this issue and it turns out that this is a bug on our part.
We did not test the code properly once it was merged through on PR.

There are two problems in the esp32s ADC driver:

  1. is wrong calculation of the adc2 channels which is solved in the same was as Fix ESP32 Lowside Current Sense ADC2 init #346
  2. is that esp32s3 has 10 adc1 channels not 7, as @mailonghua found at some point. We need to replace (channel <7) by (channel < 9).

I think that the reason that your solution worked @mailonghua (using an analogRead before the current sense init) is because the init is not done in our driver if the adc has been initalised before. So the code with the error was skipped.

The corrected code is will be available in the dev branch in the next few days.

@askuric askuric linked a pull request Jun 22, 2024 that will close this issue
@askuric
Copy link
Member

askuric commented Jul 22, 2024

Hey guys,
The fix to this issue is in the new release v2.3.4.
The esp32 driver has been rewritten and the fast ADC code stripped to the bare minimum components.

Let me know if some of you had the time to test the code and confirm that the issue is fixed.

On my esp32-s3 the issue does no longer exist.

@runger1101001 runger1101001 added this to the 2.3.4_Release milestone Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants