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

Fix broken uart module #1307

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Fix broken uart module #1307

merged 2 commits into from
Oct 10, 2024

Conversation

aiotter
Copy link
Contributor

@aiotter aiotter commented Oct 8, 2024

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

uart module for ESP32 is broken. This PR fixes it.

  • In the current code, uart:open/1,2 takes uart0, uart1 or uart2 as peripheral option. However, Nif takes either one of UART0, UART1 or UART2. This PR corrects the behaviour of uart:open to match that of Nif.
  • The Nif of uart module does not send a reply parsable by port:call, which stops the process from executing further codes. This PR fixes that by using port_send_reply function.

@aiotter aiotter changed the base branch from main to release-0.6 October 8, 2024 09:12
@aiotter
Copy link
Contributor Author

aiotter commented Oct 8, 2024

  • The Nif of uart module does not send a reply parsable by port:call, which stops the process from executing further codes.

I noticed that there was another problem on my device which prevents me from processing UART. So this statement might be false. Actually I read the old code twice and cannot find problems. It seems I have to reword the commit comment and CHANGELOG.md to state that my commit is just a refactor.

@UncleGrumpy
Copy link
Collaborator

Thank you for this! We just discovered this bug and were discussing it in our Telegram chat, which I would encourage you to join:

https://t.me/atomvm

The core developers and many community members are frequently active there, and we would welcome having you join our conversations.

@aiotter
Copy link
Contributor Author

aiotter commented Oct 9, 2024

Thank you for info about Telegram community. I already joined, but I seldom use Telegram, so I didn't noticed this PR is mentioned there.

If there is someone who wants to talk to me there, please don't hesitate to mention me. I'm there with the same username (@aiotter).

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

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

Just a couple of changes are still required. I think it can be good after that.
Let us know if the final PR works on real hardware as expected so we can merge it.
Thanks for all you did so far.

@aiotter
Copy link
Contributor Author

aiotter commented Oct 10, 2024

Thank you for your review. The current code is already tested, in other words, the commits in the PR are cherry-picked from the branch I build AtomVM from and I'm using it in my daily development.

I'll check your review, correct the code ,reword the commit comment, test it on my device and let you know then.

Use newer APIs to make it easier to read

Signed-off-by: Yuto Oguchi <[email protected]>
Nif `uart_driver` can only parse uppercased peripheral name

Signed-off-by: Yuto Oguchi <[email protected]>
@aiotter
Copy link
Contributor Author

aiotter commented Oct 10, 2024

Tested and it worked. Now it's ready to be merged.

@bettio bettio merged commit ff34010 into atomvm:release-0.6 Oct 10, 2024
86 of 91 checks passed
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.

3 participants