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

Legacy driver conflict detection mechanisms are too aggressive. #10786

Open
1 task done
lovyan03 opened this issue Dec 28, 2024 · 13 comments
Open
1 task done

Legacy driver conflict detection mechanisms are too aggressive. #10786

lovyan03 opened this issue Dec 28, 2024 · 13 comments
Labels
Area: ESP-IDF related ESP-IDF related issues

Comments

@lovyan03
Copy link

lovyan03 commented Dec 28, 2024

Board

ESP32 Dev Module

Device Description

Since this is a software issue, the hardware configuration is not particularly important.

Hardware Configuration

Since this is a software issue, the hardware configuration is not particularly important.

Version

v3.0.7

IDE Name

ArduinoIDE

Operating System

Windows11

Flash frequency

40MHz

PSRAM enabled

no

Upload speed

115200

Description

Starting with ESP-IDF v5, new drivers have appeared, including for ADC and I2S, and the previous drivers are now called legacy drivers.
Mixing new and legacy drivers can cause malfunctions, so if you use a legacy driver, an announcement will be displayed strongly recommending that you switch to the new driver.

However, I feel that the conflict detection is overly excessive. I have written some code to reproduce a specific example, so please take a look.

Sketch

1_src.ino

#include "2_base.h"

use_class_t instance;

void setup() {
  instance.virtual_function();
}

void loop() {}

2_base.h

#pragma once

struct base_t {
  virtual int virtual_function(void) = 0;
};

// This class is used at runtime.
struct use_class_t : base_t {
  int virtual_function(void) override;
};

// This class is not used at runtime.
struct no_use_class_t : base_t {
  int virtual_function(void) override;
};

3_no_use_class.cpp

/*!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
If this file exists, the project will go into a reboot loop on startup with an ADC CONFLICT error.
However, if you delete this file, the project will run normally.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!*/

#include <driver/i2s.h>

#include "2_base.h"

// This function is not called at runtime
int no_use_class_t::virtual_function(void)
{
  // For example, suppose there is a program that uses a legacy I2S driver.
  i2s_write(I2S_NUM_0, nullptr, 0, nullptr, 0);
  return 0;
}

4_use_class.cpp

#include <Arduino.h>

#include "2_base.h"

// This function is called at runtime
int use_class_t::virtual_function(void)
{
  // use the analog readout.
  return analogRead(GPIO_NUM_26);
}

Debug Message

ELF file SHA256: 2fade869ee262244

E (148) esp_core_dump_flash: Core dump flash config is corrupted! CRC=0x7bd5c66f instead of 0x0
E (156) esp_core_dump_elf: Elf write init failed!
E (161) esp_core_dump_common: Core dump write failed with error=-1
Rebooting...
ets Jul 29 2019 12:21:46

rst:0xc (SW_CPU_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0030,len:4832
load:0x40078000,len:16460
load:0x40080400,len:4
load:0x40080404,len:3504
entry 0x400805cc
E (128) ADC: CONFLICT! driver_ng is not allowed to be used with the legacy driver

abort() was called at PC 0x400d536f on core 0


Backtrace: 0x400820cd:0x3ffe3b30 0x400876b9:0x3ffe3b50 0x4008c259:0x3ffe3b70 0x400d536f:0x3ffe3bf0 0x400d795a:0x3ffe3c10 0x4008241e:0x3ffe3c40 0x400797e5:0x3ffe3c80 |<-CORRUPTED




Decoding stack results
0x400820cd: panic_abort at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/esp_system\panic.c:466
0x400876b9: esp_system_abort at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/esp_system/port\esp_system_chip.c:84
0x4008c259: abort at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/newlib\abort.c:38
0x400d536f: check_adc_oneshot_driver_conflict at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/driver/deprecated\adc_legacy.c:930
0x400d795a: start_cpu0_default at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/esp_system\startup.c:205
0x4008241e: call_start_cpu0 at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/xtensa/include\xt_utils.h:40

Other Steps to Reproduce

The important thing here is that although there are derived classes that use the legacy driver, these derived classes are not used at all.
Despite this, the project cannot be started due to a conflict check.
The reason is that the function that checks for conflicts has the __attribute__((constructor)) assigned, which forces a reboot before app_main.

Let me explain a specific situation in which this occurs.

This time, I tried to create a project using a library called ESP8266Audio.
This library contains numerous class definitions, such as the decoder part of the audio codec and the signal output part.
This library also contains a derived class for the signal output part that uses the legacy I2S driver.

I only used the decoder part of this. For the signal output part, I created my own derived class that uses the new I2S driver.

Therefore, I did not use any signal output parts that use the legacy I2S driver.
However, as shown in the example above, it failed to start.

I understand that this is a safety mechanism to prevent mixing of ESP-IDF's legacy driver and the new driver,
and that it is not essentially an issue with ArduinoESP32.
And I also understand that I should advise the ESP8266Audio repository to migrate to the new driver.
(I intend to report this to the ESP-IDF and ESP8266Audio repositories if necessary.)

However, similar problems will likely occur with other products besides ESP8266Audio.
Also, if an abort occurs immediately after startup due to a legacy driver that is not used at runtime, it can be very difficult to identify the cause.
Even if you decode the backtrace, it will be impossible to identify the cause.

Fortunately, it seems that ESP-IDF has recently added a feature to skip conflict checks for legacy drivers.
espressif/esp-idf@c80fa61

I strongly hope that this will be included in the ArduinoESP32 CONFIG.
It should be sufficient to check for conflicts with legacy drivers when initializing the legacy driver.
If a conflict error is output to the log and aborted during initialization, the cause of the problem will also be shown in the backtrace.
I expect that this will be at least more helpful for users than the current situation.

I have checked existing issues, online documentation and the Troubleshooting Guide

  • I confirm I have checked existing issues, online documentation and Troubleshooting guide.
@lovyan03 lovyan03 added the Status: Awaiting triage Issue is waiting for triage label Dec 28, 2024
@me-no-dev
Copy link
Member

Turning off the check will allow both old and new drivers to be mixed in. This could lead to unexpected results that would be hard to debug. Arduino is meant to be used not only by experienced coders but also (mostly) by unexperienced ones. I am very partial to this and generally do not agree to turn the errors off. We do provide our own I2S interface that the libraries can use to interface with the bus. Why not use that instead and stay compatible from now on?

@lovyan03
Copy link
Author

Sorry, I agree with the argument for moving to the new drivers, and I think it would be great if all open source libraries registered with Arduino Manager were compatible with the new drivers right now.

I'm probably not quite clear on what I'm trying to say, so let me explain a bit more.

  • There is no intention to use the old and new drivers at the same time, and the old drivers are not used in the above example.

  • The above example is actually included in a library written by someone else and is not used at runtime, meaning that the legacy driver is not used at runtime at all.

  • This means that the above example will force a reboot before app_main, even though it is actually safe to run.

  • If using them simultaneously is a problem, it will be easier to follow the backtrace if you check for conflicts when initializing the driver and abort the process.

  • If the program is forced to abort before app_main, it is highly unlikely that an inexperienced programmer would be able to debug it. They would have no idea what the cause is.

@Jason2866
Copy link
Collaborator

Jason2866 commented Dec 28, 2024

Imho there will be situations you can not "guard" correctly when trying to use new drivers and using libs which are using deprecated drivers and hacking in support for new stuff.
So the clean way change the used libs to support both correctly or only the new one.
In a not to far future espressif Arduino will move on to IDF 5.4, and deprecated drivers are removed.
Btw ESP8266Audio needs a few more changes to work more or less well with all actual esp32x MCUs.

@lovyan03
Copy link
Author

Yes, I completely agree.
My hope is not to make new libraries less secure.

Here's another example:

#include <Arduino.h>
#include <driver/i2s.h>

void no_use_function(void)
{
  i2s_write(I2S_NUM_0, nullptr, 0, nullptr, 0);
}

void setup() {
  analogRead(26);
}

void loop() {}

This program is forced to abort due to a driver conflict check, and app_main doesn't even start.

  • We should not make it less secure for new libraries.
  • However, it is excessive defense to not even start the program just because some libraries contain code that is not used at runtime.
  • The timing of the conflict check is the problem. It is enough to abort when the initialization function of the old library is called.

I believe that libraries that depend on old drivers and are not updated should be discarded, but not all libraries in the world are stably maintained. In other words, during the transition period, new and old drivers are linked together.

@SuGlider
Copy link
Collaborator

This is similar to this issue (related to legacy RMT IDF driver):
#9866 (comment)

E (91) rmt(legacy): CONFLICT! driver_ng is not allowed to be used with the legacy driver
abort() was called at PC 0x4200951f on core 0

@lovyan03
Copy link
Author

This is similar to this issue (related to legacy RMT IDF driver): #9866 (comment)

Yes, thanks.

Essentially, if check for the presence of the new driver during the init of the legacy driver and abort if mixing is detected, no conflicts will occur.
Nevertheless, forcing an abort before app_main is overly defensive.

@SuGlider
Copy link
Collaborator

Yes, I completely agree. My hope is not to make new libraries less secure.

Here's another example:

#include <Arduino.h>
#include <driver/i2s.h>

void no_use_function(void)
{
  i2s_write(I2S_NUM_0, nullptr, 0, nullptr, 0);
}

void setup() {
  analogRead(26);
}

void loop() {}

This program is forced to abort due to a driver conflict check, and app_main doesn't even start.

  • We should not make it less secure for new libraries.
  • However, it is excessive defense to not even start the program just because some libraries contain code that is not used at runtime.
  • The timing of the conflict check is the problem. It is enough to abort when the initialization function of the old library is called.

I believe that libraries that depend on old drivers and are not updated should be discarded, but not all libraries in the world are stably maintained. In other words, during the transition period, new and old drivers are linked together.

This issue lays in ESP32 only and within IDF 5.1 and IDF 5.3 (used by Arduino Core 3.0.x and 3.1.0).
ESP32 IDF Code from #include <driver/i2s.h> will also include #include "driver/adc. h"
Therefore, yes, it is HW dependent, because ESP32 I2S can be used for ADC reading.

Running the same code using ESP32-S2 or ESP32-S3 (or any RISC-V -> C3, C6, H2), it doesn't fail.

@SuGlider
Copy link
Collaborator

@lovyan03 - I really love your work with your library, but in this case Arduino Framework can't help it.
ESP32 Arduino Framework is IDF dependent. All the issues and fails you are reporting here are in the IDF code that we can't change.

Let us know what would be your suggestion about how Arduino could fix it.

@lovyan03
Copy link
Author

YES! AGREE!!
I'm very relieved that you understand what I'm saying.
As you pointed out, this issue is essentially an ESP-IDF issue. I initially considered writing this in the ESP-IDF repository, but decided to write it in the ArduinoESP32 repository first.
This is because ESP-IDF is used by more experienced users than ArduinoESP32, and if you use it foolishly, such as mixing it with legacy drivers, you will be laughed at and that's the end of it.
However, there are actual examples of problems with ArduinoESP32, and I'm sure you will understand that this is a trap that leads beginners into a maze. If I could get agreement here, I was planning to propose it in the ESP-IDF repository with that fact in mind.

@lovyan03
Copy link
Author

What I didn't anticipate was that there were people who firmly believed that the current conflict checking system was perfect to protect beginners.

@SuGlider
Copy link
Collaborator

Summary:

IDF Legacy Driver for any peripheral is incompatible with the new IDF 5.x same peripheral driver.
It is not about beginner or experienced developers. It is about making IDF work correctly.

Arduino 3.x has its API implementation based on IDF 5.
Using any Arduino API will activate the new IDF driver.

Therefore, using Legacy Drivers shall be done exclusively.

The issue here presented: ESP32 SoC using IDF Legacy I2S and Arduino ADC in the same code.
Whenever I2S IDF Legacy Driver is included, it will include ADC Legacy Driver as well, for the ESP32 SoC only.
Therefore, both, I2S and ADC legacy drivers will be present in the same code.

In order to fix it, the suggestion is to use ADC Legacy IDF calls to read Analog signals, instead of using Arduino Analog Read API.

@lovyan03
Copy link
Author

lovyan03 commented Dec 31, 2024

Yes, if the new driver is linked, it's okay if the old driver is not available.
However, this only needs to be done at driver initialization time, and there is no need to force it to be done before app_main.

If you are using only the legacy I2S driver for normal I2S communication on a normal ESP32, the legacy ADC driver should not initialized, which means the code above should be safe to run.

In other words, if we try to do an analog read with a legacy I2S driver, it should be aborted during driver initialization.

@SuGlider
Copy link
Collaborator

SuGlider commented Jan 1, 2025

Yes, if the new driver is linked, it's okay if the old driver is not available.
However, this only needs to be done at driver initialization time, and there is no need to force it to be done before app_main.

If you are using only the legacy I2S driver for normal I2S communication on a normal ESP32, the legacy ADC driver should not initialized, which means the code above should be safe to run.

In other words, if we try to do an analog read with a legacy I2S driver, it should be aborted during driver initialization.

It sounds to be the best way to go.
This suggestion shall be done within ESP-IDF github.

@SuGlider SuGlider added Area: ESP-IDF related ESP-IDF related issues and removed Status: Awaiting triage Issue is waiting for triage labels Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: ESP-IDF related ESP-IDF related issues
Projects
None yet
Development

No branches or pull requests

4 participants