lovyan03 / LovyanGFX

SPI LCD graphics library for ESP32 (ESP-IDF/ArduinoESP32) / ESP8266 (ArduinoESP8266) / SAMD51(Seeed ArduinoSAMD51)
Other
1.03k stars 189 forks source link

ESP-32 build error: no member named 'comd' #438

Closed hayschan closed 4 months ago

hayschan commented 9 months ago

Environment ( 実行環境 )

Hello. I'm reporting because I think I found a bug.

I think this may be related to the fix published about the ESP-IDF error mentioned here. It might cause a new bug. https://github.com/lovyan03/LovyanGFX/pull/423

I'm using ESP32-S3 and I wrote the code with ESP-IDF v5.0.2.

Symptom

The error codes are as follows.

./components/LovyanGFX/src/lgfx/v1/platforms/esp32/common.cpp: In function 'void lgfx::v1::i2c::i2c_set_cmd(i2c_dev_t*, uint8_t, uint8_t, uint8_t, bool)':

./components/LovyanGFX/src/lgfx/v1/platforms/esp32/common.cpp:808:14: error: 'struct i2c_dev_t' has no member named 'comd'; did you mean 'comd0'?
  808 |       (&dev->comd[0])[index].val = cmd_val;
      |              ^~~~
      |              comd0

My fix

When I change the line of code from (&dev->comd[0]) to (&dev->comd0), the build errors disappear.

Before:

// common.cpp:806
#if defined (CONFIG_IDF_TARGET_ESP32S3)
 #if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 0))
      (&dev->comd[0])[index].val = cmd_val;
 #else
      (&dev->comd0)[index].val = cmd_val;
 #endif
#else

After:

// common.cpp:806
#if defined (CONFIG_IDF_TARGET_ESP32S3)
 #if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 0))
      (&dev->comd0)[index].val = cmd_val;
 #else
      (&dev->comd0)[index].val = cmd_val;
 #endif
#else
lovyan03 commented 9 months ago

Thanks for reporting back. It is necessary to identify exactly from which version of the IDF the notation has changed.

hayschan commented 9 months ago

My version is v5.0.2.

> idf.py --version
ESP-IDF v5.0.2-dirty
lovyan03 commented 9 months ago

Basically, only the released version is supported. I cannot guarantee the operation of other versions.

hayschan commented 9 months ago

~(Fuc* ESP)~ 😠

I will track down every major version, and help with creating a detailed #if #else statements regarding every major release version. I'll fork, contribute and pull request soon. Let me know if you think this will work.

v5.0.3

    volatile i2c_comd_reg_t comd[8];
    // ...
} i2c_dev_t;

Link to code: https://github.com/espressif/esp-idf/blob/v5.0.3/components/soc/esp32s3/include/soc/i2c_struct.h#L996

v5.0.2

    volatile i2c_comd0_reg_t comd0;
    // ...
} i2c_dev_t;

Link to code: https://github.com/espressif/esp-idf/blob/v5.0.2/components/soc/esp32s3/include/soc/i2c_struct.h#L1161

tobozo commented 9 months ago

@hayschan can you try with this macro?

#if defined (CONFIG_IDF_TARGET_ESP32S3)
 #if (ESP_IDF_VERSION > ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 0))
      (&dev->comd[0])[index].val = cmd_val;
 #else
      (&dev->comd0)[index].val = cmd_val;
 #endif
#else
hayschan commented 9 months ago

Yep, it works, as I would be running the #else statement part.

hayschan commented 9 months ago

comd, comd[] in every major ESP-IDF version

I just checked ESP-IDF every major version commit tags. In v5.1, it changes back to comd0 again. 😕 Only a single ESP-IDF version v5.0.3 uses comd[8].

image

It seems that comd[8] comes out of no where.

Version Variable type Link to reference (to the exact line)
v5.1 comd0 esp-idf/components/soc/esp32s3/include/soc/i2c_struct.h at v5.1 · espressif/esp-idf (github.com)
v5.0.3 comd[8] esp-idf/components/soc/esp32s3/include/soc/i2c_struct.h at v5.0.3 · espressif/esp-idf (github.com)
v5.0.2 comd0 esp-idf/components/soc/esp32s3/include/soc/i2c_struct.h at v5.0.2 · espressif/esp-idf (github.com)
v4.4.5 comd0 esp-idf/components/soc/esp32s3/include/soc/i2c_struct.h at v4.4.5 · espressif/esp-idf (github.com)
v4.3.5 N/A, does not exist this variable esp-idf/components/soc/esp32s3/include/soc/i2c_struct.h at v4.3.5 · espressif/esp-idf (github.com)

Possible fix suggestion

As only a single ESP-IDF version v5.0.3 changes to comd[8], we could just make a single #if targeting this version.

#if defined (CONFIG_IDF_TARGET_ESP32S3)
 #if (ESP_IDF_VERSION == ESP_IDF_VERSION_VAL(5, 0, 3))
      (&dev->comd[0])[index].val = cmd_val;
 #else
      (&dev->comd0)[index].val = cmd_val;
 #endif
#else
tobozo commented 9 months ago

esp-idf sure provides a lot of efforts to scare away maintainers ... hopefully their inclusion in the Matter project will break a lot of eggs with backporting and bring instant karma to whomever dictated such a schizophrenic strategy

lovyan03 commented 9 months ago

I would like to consider the following possibilities

Perhaps it might be faster to access the register using the address definition instead of using the ESP-IDF structure definition to solve the problem.

smeisner commented 8 months ago

Using the latest (as far as I can tell) ESP IDF (v5.1.1) on an ESP32-S3, the only way I could get this to work was to change the second test to be less than 5.1.2 ... as follows:

// common.cpp:806
#if defined (CONFIG_IDF_TARGET_ESP32S3)
 #if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 2))
      (&dev->comd0)[index].val = cmd_val;
 #else
      (&dev->comd0)[index].val = cmd_val;
 #endif
lovyan03 commented 8 months ago

@smeisner The content after the conditional branching seems to be exactly the same?

smeisner commented 8 months ago

@lovyan03 Notice I changed the test from "5, 1, 0" to "5, 1, 2"

lovyan03 commented 8 months ago

I have fixed the develop branch, please try it and see.

smeisner commented 8 months ago

I Just did a fresh build with your change (I copied and pasted the modification) and it worked perfectly.

dk307 commented 8 months ago

5.1.1 again uses comd, so breaks again for 5.1.1

https://github.com/espressif/esp-idf/blob/v5.1.1/components/soc/esp32s3/include/soc/i2c_struct.h#L996

lovyan03 commented 8 months ago

Understood. I will confirm after a new version of ESP-IDF is officially released. I will not support a version that has not yet been released.

lovyan03 commented 8 months ago

https://github.com/lovyan03/LovyanGFX/blob/develop/src/lgfx/v1/platforms/esp32/common.cpp#L810-L811

dk307 commented 8 months ago

Understood. I will confirm after a new version of ESP-IDF is officially released. I will not support a version that has not yet been released.

5.1.1 has been released. https://github.com/espressif/esp-idf/releases/tag/v5.1.1

emaayan commented 8 months ago

so if i undestand correctly this library currently simply isn't compatible with version 5.0.3 right? (i don't mean it as a bad thing just wanted to confirm, i've been looking hopelessly for a working example that shows to use T display cards and i have yet to find any., so i'm willing even to use CPP in a C project if that works

hayschan commented 8 months ago

so if i undestand correctly this library currently simply isn't compatible with version 5.0.3 right?

It works perfectly most of the time.

When you encounter the problem mentioned in this issue, you just have to change the line according to the reply above.

lovyan03 commented 8 months ago

If I understand correctly, the develop branch supports ESPIDF v5.0.3 This will be reflected in the next release.

github-actions[bot] commented 7 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

JeremiahGillis commented 6 months ago

Just bumping this thread since issue is still present for v5.1.1 which is released. I just encountered this today when upgrading my IDF version from v5.1.

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

JeremiahGillis commented 5 months ago

Bumping to keep issue open.

github-actions[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

devalnor commented 4 months ago

I use ESP-IDF 5.1.1 via PIO Espressif 32 6.4.0 I just changed in common.cpp and it work but not sure if it's the best move.


 #if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 2))
      (&dev->comd[0])[index].val = cmd_val;
 #else
      (&dev->comd0)[index].val = cmd_val;
 #endif
#else
      dev->command[index].val = cmd_val;
#endif```
andrewjjenkins commented 4 months ago

Almost the same for me as @devalnor , but now there is esp-idf 5.1.2 and it has dev->comd[0]. So I need to bump the uppermost version again:

$ git diff
diff --git a/src/lgfx/v1/platforms/esp32/common.cpp b/src/lgfx/v1/platforms/esp32/common.cpp
index c0b513c..87f8079 100644
--- a/src/lgfx/v1/platforms/esp32/common.cpp
+++ b/src/lgfx/v1/platforms/esp32/common.cpp
@@ -806,7 +806,7 @@ namespace lgfx
         cmd_val |= (1 << 10); // ACK_VALUE (set NACK)
       }
 #if defined (CONFIG_IDF_TARGET_ESP32S3)
- #if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 0))
+ #if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 2) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 3))
       (&dev->comd[0])[index].val = cmd_val;
  #else
       (&dev->comd0)[index].val = cmd_val;

Maybe the best fix is to remove the uppermost version check, and assume all versions after 5.0.2 will use dev->comd[0]?

tobozo commented 4 months ago

@andrewjjenkins 5.1.2 works fine with the current develop branch, 5.0.5 and 4.4.6 are supported too

checking the idf version from a multi conditional macro may not be such a good idea, a more elegant solution should be found

andrewjjenkins commented 4 months ago

Oh I see the new condition check, thanks @tobozo . I was not checking the develop branch.

tobozo commented 3 months ago

bump

esp-idf 5.2 is now officially the latest release.

Some macros needed to be adjusted in LovyanGFX source tree to fix the compilation errors, the changes are available on the develop branch

image