[BUG] ESP32-S3 Stuck in adc functions

CommanderRedYT commented 11 months ago

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): https://github.com/simplefoc/Arduino-FOC/blob/05954cb9691534dd478407505d810a68f1673a5e/src/current_sense/hardware_specific/esp32/esp32s_adc_driver.cpp#L237-L243

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

Visual Studio Code with Platformio
espressif32@6.3.2 (latest)

[env:esp32-s3-devkitc-1]
platform = espressif32
board = esp32-s3-devkitc-1
framework = arduino
lib_deps = askuric/Simple FOC@2.3.0
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 = espressif32@2.0.0
board = esp32dev
framework = arduino
lib_deps = askuric/Simple FOC@2.2.0
lib_archive = false
monitor_speed = 115200
upload_speed = 921600

runger1101001 commented 10 months ago

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

mailonghua commented 9 months ago

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

CommanderRedYT commented 9 months ago

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

mailonghua commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

@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 commented 7 months ago

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 commented 7 months ago

We have just merged this PR: https://github.com/simplefoc/Arduino-FOC/pull/346 to the dev branch.

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

CommanderRedYT commented 7 months ago

We have just merged this PR: https://github.com/simplefoc/Arduino-FOC/pull/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 commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 1 week ago

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:

is wrong calculation of the adc2 channels which is solved in the same was as #346
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.

simplefoc / Arduino-FOC

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