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

Rework of UART on MIMXRT1060 #1529

Merged
merged 2 commits into from
Dec 16, 2019
Merged

Conversation

morali
Copy link
Contributor

@morali morali commented Dec 13, 2019

It's cleanup of code and rework of some of the functions. I think that read and write is now more stable and it shouldn't drop any frames, like it with old implementation.

Description

Motivation and Context

More optimised code, less variables, use of static variables, checks for NULL pointers.

How Has This Been Tested?

Tested with SerialCommunication project from nanoFramework Samples.

I've tested read and write on LPUART3 port. I could receive and display strings on terminal properly.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: morali jeremi.jasinski@gmail.com

@nfbot
Copy link
Member

nfbot commented Dec 13, 2019

Hi @morali,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

@josesimoes josesimoes added Series: NXP i6 Everything related specifically with NXP targets Type: enhancement labels Dec 13, 2019
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Overall looks good! 👏
I can't test it so I'll default to your judgment on that.

Have to ask you to update the coding style to match the rest of the repo.
(we should have a linter to auto-format but we still don't have it, sorry)

Namelly:

  • comments with // instead of /* */
  • if clauses like this
if (condition)
{
}
else
{
}

/* check if all requested bytes were written */
if (bytesWritten != length) {
/* not sure if this is the best exception to throw here... */
NANOCLR_SET_AND_LEAVE(CLR_E_FAIL);
Copy link
Member

Choose a reason for hiding this comment

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

This one is meant for a general CLR failure!! Too drastic to use here.
Maybe CLR_E_IO...

@morali
Copy link
Contributor Author

morali commented Dec 13, 2019

Thank you for your insight.
I forgot about the coding style, we use tool for auto-format. It would be nice if nf could adapt some tool.
I will fix coding style and replace exception with different one as you suggested. :)

@josesimoes
Copy link
Member

@morali

I forgot about the coding style, we use tool for auto-format. It would be nice if nf could adapt some tool.

I hear you! That's on the todo list for quite sometime... 😅

Would like to find a tool that could work both locally and could be launched as a GitHub check or even Azure Pipeline.

Any suggestions?

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM!

@josesimoes
Copy link
Member

@morali

I forgot about the coding style, we use tool for auto-format. It would be nice if nf could adapt some tool.

I hear you! That's on the todo list for quite sometime... 😅

Would like to find a tool that could work both locally and could be launched as a GitHub check or even Azure Pipeline.

Any suggestions?

@josesimoes josesimoes merged commit 9674f93 into nanoframework:develop Dec 16, 2019
@coderabbitai coderabbitai bot mentioned this pull request Dec 20, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Series: NXP i6 Everything related specifically with NXP targets Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants