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

Esp32 Core version 3 #2144

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

BorisKofman
Copy link
Contributor

@BorisKofman BorisKofman commented Sep 6, 2024

Fixes #2039

@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 6, 2024

Duplicate of #2040 / #2039 ?

But please provide an explanation if this has any improvements over the other.

@BorisKofman BorisKofman closed this Sep 8, 2024
@BorisKofman BorisKofman deleted the Espressif-version-3 branch September 8, 2024 11:09
@BorisKofman BorisKofman restored the Espressif-version-3 branch September 8, 2024 11:19
@BorisKofman BorisKofman reopened this Sep 8, 2024
@BorisKofman
Copy link
Contributor Author

Feature:

Make IRremoteESP8266 compatible with IDF 5.x ESP32 Version 3

@BorisKofman
Copy link
Contributor Author

BorisKofman commented Sep 8, 2024

@NiKiZe code is running on ESP32 S3 With ESP32 Version 3.0.4 without any errors

I will try to test with ESP32-DEV & ESP32-C6 this weak

@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 8, 2024

As I understand it, so is #2040, we are grateful for your PR. Just want to limit duplication if there already is a fully working implementation.

@BorisKofman
Copy link
Contributor Author

Thanks @NiKiZe,
With #2040 I get error: timer is not enabled yet
I will try to test my Code with IDF 5.x and much Boards that I can
Currently tested "IDF 5.X" with S3.
Tomorrow will try with C6 and esp32-dev.
Then I will revert to ESP32 board version 2 to test again with S3 & DEV boards
Thanks!

@Jason2866
Copy link

#2040 is working perfectly fine.

@Buddy-Matt
Copy link

Buddy-Matt commented Sep 16, 2024

@Jason2866 - #2040 isn't working on the M5Stack NanoC6 under Arduino IDE. Have commented on the relevant PR

I'm able to decode my remote using this pull request

@BorisKofman
Copy link
Contributor Author

@Jason2866 Tested with ESP32-C6 WROOM1, ESP32 WROVER and the code is working on Esp32 Core version 3.
Thanks!

@BorisKofman BorisKofman requested a review from NiKiZe September 17, 2024 10:54
@BorisKofman
Copy link
Contributor Author

@NiKiZe @Jason2866 Thank you for feedback did the required changes
Thanks!

NiKiZe added a commit to NiKiZe/IRremoteESP8266 that referenced this pull request Sep 17, 2024
NiKiZe added a commit to NiKiZe/IRremoteESP8266 that referenced this pull request Sep 17, 2024
@NiKiZe NiKiZe requested a review from crankyoldgit September 23, 2024 12:33
@NiKiZe NiKiZe force-pushed the Espressif-version-3 branch from e23c19e to f858d28 Compare September 23, 2024 14:48
@BorisKofman
Copy link
Contributor Author

Hi @NiKiZe @crankyoldgit @Jason2866,
I in middle of vacation and can't update your request.
Maybe someone please can finish it or I will try to finishing next weak.
Thanks!

@BorisKofman
Copy link
Contributor Author

@NiKiZe @crankyoldgit I made changes as you request not if failing on code Lint but not sure if it because of me.

@hendriksen-mark
Copy link

This works for me on wemos d1 mini32.

@Ks89
Copy link

Ks89 commented Jan 10, 2025

Are there any news about the merge of this PR?
Thanks

@robertlipe
Copy link

Are there any news about the merge of this PR? Thanks

Those of us needing current support (including a compiler from the last 7 years and any of the newer Espressif parts) may need to start looking at libraries like https://github.com/Arduino-IRremote/Arduino-IRremote which added this support many months ago. This (and the similarly needed #2040) have been in the review queue with no visible motion forward for a long time.

zehnm pushed a commit to zehnm/IRremoteESP8266 that referenced this pull request Feb 4, 2025
zehnm added a commit to zehnm/IRremoteESP8266 that referenced this pull request Feb 5, 2025
Include open upstream PR crankyoldgit#2144
to make IR learning compatible with IDF 5.x.

Enhance with:
* update esp32-hal* to IDF v5.4
* C++20 compatibility
  A small fix to remove "volatile ++" build warning.
  Source: crankyoldgit#2040

---------

Co-authored-by: BorisKofman <[email protected]>
Co-authored-by: Christian I. Nilsson <[email protected]>
@robertlipe
Copy link

Is this project still active? #2039
#2040
#2055
#2107
#2122
#2126
#2137
#2141
#2157
#2169
#2178
#2185
#2188
#2189
all more or less point to here,
#2144

The maintainer(s) (Are NikiZe and crankyolddigit the same person?) are clearly getting cranky with people not finding this, but fixes for this problem seem to have been around for over a year now. Now that Pioarduino has broken the Arduino3 logjam for more devs/users, the number of affected people will only grow, further amplifying the feedback loop.

It's been confirmed to work. I haven't seen that the patches are pending further changes from authors. What will it take to get these changes in the hands of the people that need them?

Different patches have had some additional stuff around them, but the actual timer changes in ESP-IDF seem pretty mechanical. Yes, it's annoying for Espressif to break our code renaming our symbols, but we can't control that and the rest of our code was broken by similary annoying changes in Arduino3.

I searched GitHub to see if someone had just forked it with these changes applied, but they don't seem to have done so. I don't particularly want to become a project maintainer of a fork of this. (My requirements are low. I need to receive NEC; I can probably pluck the IR frames right off the RMT via ESP-IDF, I think) I don't want to patch PlatformIO and then tell our developers where to find a crossplatform version of patch to apply it.

If it "just" needs some further programmer attention, I'll block off some time and try to help. I can't help test a zillion remotes I've never heard of. Honestly, I'd rather spend two hours replacing this than an hour fixing this at this point, but not everyone has that luxury.

Please consider applying one of the above fixes for this problem for later ESP-IDF - which is also breaking Arduino/ESP32 and pushing it out as a release. That'll reduce/stop the flow of incoming duplicate reports. I'm willing to help a little, but I don't want to take over a fork of the project.

Thank you for listening. I hope we can get a delivery of this soon.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Feb 27, 2025

Is this project still active? #2039 #2040 #2055 #2107 #2122 #2126 #2137 #2141 #2157 #2169 #2178 #2185 #2188 #2189 all more or less point to here, #2144

The maintainer(s) (Are NikiZe and crankyolddigit the same person?) are clearly getting cranky with people not finding this, but fixes for this problem seem to have been around for over a year now. Now that Pioarduino has broken the Arduino3 logjam for more devs/users, the number of affected people will only grow, further amplifying the feedback loop.

It's been confirmed to work. I haven't seen that the patches are pending further changes from authors. What will it take to get these changes in the hands of the people that need them?

Different patches have had some additional stuff around them, but the actual timer changes in ESP-IDF seem pretty mechanical. Yes, it's annoying for Espressif to break our code renaming our symbols, but we can't control that and the rest of our code was broken by similary annoying changes in Arduino3.

I searched GitHub to see if someone had just forked it with these changes applied, but they don't seem to have done so. I don't particularly want to become a project maintainer of a fork of this. (My requirements are low. I need to receive NEC; I can probably pluck the IR frames right off the RMT via ESP-IDF, I think) I don't want to patch PlatformIO and then tell our developers where to find a crossplatform version of patch to apply it.

If it "just" needs some further programmer attention, I'll block off some time and try to help. I can't help test a zillion remotes I've never heard of. Honestly, I'd rather spend two hours replacing this than an hour fixing this at this point, but not everyone has that luxury.

Please consider applying one of the above fixes for this problem for later ESP-IDF - which is also breaking Arduino/ESP32 and pushing it out as a release. That'll reduce/stop the flow of incoming duplicate reports. I'm willing to help a little, but I don't want to take over a fork of the project.

Thank you for listening. I hope we can get a delivery of this soon.

Fix the name, essentially replace the last commit here, and ensure there is no limit issues.
Minimize the PR to just timer changes, nothing else.
Any other changes needs their own proper descriptions and not mixed in to the rest.

Copy link

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a swapped comment pair, this looks pretty minimal to me. The only things that aren't strictly minimal were requested and implemented in earlier rounds of review.

If there's something specific that remains, please highlight it. I don't want to scoop the author, but if there's something more to do here, I'll help if needed.

Seems it's been pretty widely tested by many of us already.

Thank you for the quick pickup.

src/IRrecv.cpp Outdated
timer = timerBegin(_timer_num, 80, true);
// Check for ESP32 core version and handle timerBegin differently for each version
#if defined(ESP_ARDUINO_VERSION) && (ESP_ARDUINO_VERSION >= ESP_ARDUINO_VERSION_VAL(3, 0, 0))
// For ESP32 core version 3.x (three arguments)
Copy link

@robertlipe robertlipe Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think last half of this comment and the one on 377 are swapped.

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

Successfully merging this pull request may close these issues.

Feature Request: Support for esp32 Arduino 3.0.0
8 participants