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

ESP-IDF support #745

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

Conversation

fedepell
Copy link

Adds support for serial for ESP-IDF and introduces a document on how to use it in that environment.

Assumes to be merged after #741 (I will rebase accordingly in case).

Copy link

cla-bot bot commented May 20, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

1 similar comment
Copy link

cla-bot bot commented Jun 29, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

stephane added a commit that referenced this pull request Jul 16, 2024
stephane added a commit that referenced this pull request Jul 16, 2024
@diplfranzhoepfinger
Copy link
Contributor

sorry, i overlooked this !

@fedepell
Copy link
Author

sorry, i overlooked this !

No problem :) Sorry for my delay now, was on a short vacation ;)

I hope we can get this (ESP support in general, in some variant) in as it may be very handy to have it mainstreamed. Also I've done a lot of testing with my code (both serial and TCP) and works like a charm (while the official internal library has a lot of limitations, ie. trivially missing possibility to have multiple connections which is just addressed in the beta).

@robin-twice
Copy link

@fedepell Thanks for your efforts with the ESP-IDF platform support!

I've tried to use your implementation as well, but stumbled upon some issues and wanted to asked whether you can confirm that: Using the ESP-IDF UART VFS with Modbus RTU, the uart_read() function does return early on some special characters (0x0A, 0x0D) depending on the line endings, which then creates either CRC errors or incomplete messages thereafter:

2024-10-07 07:23:09 Waiting for a confirmation...
2024-10-07 07:23:09 <47><03><02><00><0D><0A><B1>
2024-10-07 07:23:09 ERROR CRC received 0xB10A != CRC calculated 0x4EF0
2024-10-07 07:23:10 [47][03][9C][98][00][01][25][13]
2024-10-07 07:23:10 Waiting for a confirmation...
2024-10-07 07:23:10 <8C><47><03><02><00>
2024-10-07 07:23:10 Request for slave 140 ignored (not 71)
2024-10-07 07:23:10 The responding slave 140 isn't the requested slave 71

I've found a ticket on the ESP-IDF project where this issue with VFS UART read() behaviour is being addressed:
espressif/esp-idf#14155

Would be interested in your opinion about that and whether you've seen similar issues.

@stephane
Copy link
Owner

Could you rebase your draft PR against master branch, please?

@fedepell
Copy link
Author

Could you rebase your draft PR against master branch, please?

Rebased on master and squashed commits to one!

@fedepell
Copy link
Author

@robin-twice : sorry for the late response, very busy period :(

I've noticed some seldom "bad crc" messages, but didn't investigate in big detail as I saw (I did many long term tests and kept statistics of errors by type and so on) the same (very low) amount, actually even slightly higher, using the ESP native modbus library (which may suffer from the same problem if it reads data same way?) which I compared to (so I assumed it was just my lab-grade cabling or the 485 level adapter I have flying).

I'll try to give it a deeper look ASAP, hopefully this weekend or next week!

@fedepell fedepell marked this pull request as ready for review October 23, 2024 03:11
@fedepell
Copy link
Author

I just pushed a few moments ago two little changes:

  • use uart_vfs_dev_use_driver instead of esp_vfs_dev_uart_use_driver as the latter is deprecated and resulted in a warning
  • Add in the docs also a note of having possibly (if console UART is disabled, something I faced today ;-) ) to explicitly initialize the VFS in your code if needed (I would not put this inside libmodbus as it is a system wide decision) with uart_vfs_dev_register (see:
    solution. Be aware that if you are not using the UART0 as console (ie. you are
    )

@fedepell
Copy link
Author

fedepell commented Nov 19, 2024

@fedepell Thanks for your efforts with the ESP-IDF platform support!

I've tried to use your implementation as well, but stumbled upon some issues and wanted to asked whether you can confirm that: Using the ESP-IDF UART VFS with Modbus RTU, the uart_read() function does return early on some special characters (0x0A, 0x0D) depending on the line endings, which then creates either CRC errors or incomplete messages thereafter:

2024-10-07 07:23:09 Waiting for a confirmation...
2024-10-07 07:23:09 <47><03><02><00><0D><0A><B1>
2024-10-07 07:23:09 ERROR CRC received 0xB10A != CRC calculated 0x4EF0
2024-10-07 07:23:10 [47][03][9C][98][00][01][25][13]
2024-10-07 07:23:10 Waiting for a confirmation...
2024-10-07 07:23:10 <8C><47><03><02><00>
2024-10-07 07:23:10 Request for slave 140 ignored (not 71)
2024-10-07 07:23:10 The responding slave 140 isn't the requested slave 71

I've found a ticket on the ESP-IDF project where this issue with VFS UART read() behaviour is being addressed: espressif/esp-idf#14155

Would be interested in your opinion about that and whether you've seen similar issues.

Sorry it took forever to me to test :-( ($DAYJOB priorities ;-) )

I partially confirm what you found: to reproduce I crafted requests of 10 registers (so a 0x0a gets there with no doubts) and could reproduce, the write was not going through.

I've then added (see last pushed commit) the ESP_LINE_ENDING_LF configuration (to both read and write) and that seems to be gone now. And now I don't get also that seldom errors I saw (interesting enough the same problem was in the ESP internal modbus protocol, so that mislead me :-( ).

But as I see here in libmodbus we are anyway select()-ing and read()-ing until a certain length we want (or timeout of course) so we are anyhow looping on it (as in "alternatives you have considered" section in the ticket you linked to), so I'm not sure the problem should apply?

If it should, it seems that the solution was finally merged in master (espressif/esp-idf@92b4c62#diff-8f70148cf331b8d8293e6c953cf8d7ce6a5951cb0ceb6c087ffd9c176aabc292R172) so should be out with next release, so maybe still not worth doing further workarounds? (what do you say @stephane ?)

Many thanks for your feedback and sorry again for the very late feedback :(

@robin-twice
Copy link

@fedepell No worries, thanks for coming back to it :)
ESP_LINE_ENDING_LF did help somewhat for me, but there were still some cases where it returned early (can't remember exactly whether on 0x0A or 0x0D).

Anyway, I also agree that with espressif/esp-idf#14155 fixed it should not be an issue anymore, I haven't tested it myself though. Maybe you could also add to the readme that it needs >= xx.xx release of ESP-IDF to properly work.

By default some line ending conversion may be done, so make sure to
disable this on the UART used with libmodbus.
@fedepell
Copy link
Author

fedepell commented Nov 19, 2024

@fedepell No worries, thanks for coming back to it :) ESP_LINE_ENDING_LF did help somewhat for me, but there were still some cases where it returned early (can't remember exactly whether on 0x0A or 0x0D).

Anyway, I also agree that with espressif/esp-idf#14155 fixed it should not be an issue anymore, I haven't tested it myself though. Maybe you could also add to the readme that it needs >= xx.xx release of ESP-IDF to properly work.

Added a few lines to address this in ESP specific README heads-up and a reference to the ticket, good to save possible headaches to some future user :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants