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

Non-blocking functions #50

Merged
merged 9 commits into from
Aug 9, 2024
Merged

Conversation

adnan-ashraf
Copy link
Contributor

Provided functions enabling non-blocking operation (closes #49)

@adnan-ashraf adnan-ashraf force-pushed the non-blocking-functions branch from 68e54cd to 727c57a Compare May 20, 2024 19:00
@adnan-ashraf
Copy link
Contributor Author

Can someone please review this pull request? @Letme

Copy link
Member

@Letme Letme left a comment

Choose a reason for hiding this comment

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

Nice contribution. Thanks for the effort. I just have few stylistic requests.

Sorry it took so long, I must have missed this somehow.

src/mlx90632.c Outdated Show resolved Hide resolved
src/mlx90632.c Outdated Show resolved Hide resolved
src/mlx90632.c Outdated Show resolved Hide resolved
src/mlx90632.c Outdated Show resolved Hide resolved
@Letme
Copy link
Member

Letme commented Aug 5, 2024

Ok, now CI is also running. Can I ask you to also add some tests for your changes, to make sure we do not break anything (and that we did not break anything now)?

@Letme
Copy link
Member

Letme commented Aug 5, 2024

Regarding CI: I need to fix master, but I am waiting for Ceedling 1.0.0 release (this current version in master is fairly close to the 1.0.0, so it should be easy). That means that currently unit tests are not running as we are referencing non-existing pre-release https://github.com/ThrowTheSwitch/Ceedling/releases

@adnan-ashraf adnan-ashraf force-pushed the non-blocking-functions branch from 09b871d to fe7017a Compare August 6, 2024 09:40
@adnan-ashraf adnan-ashraf force-pushed the non-blocking-functions branch from fe7017a to 7316ce0 Compare August 6, 2024 09:55
@adnan-ashraf
Copy link
Contributor Author

Thanks for taking the time to review!

Appreciate the feedback! Your suggestions have been added.

Sure, I'll write the tests.

@yogeshiggalore
Copy link

when this will get merged..?

@Letme
Copy link
Member

Letme commented Aug 8, 2024

As soon as some tests are added to maintain functionality. I will do a local run with a latest pre-release to make sure that after fix of CI we have all green checks.

Before for some reason CI was not run on external pull requests, but now we fixed this. The only problem is that Ceedling did not release 1.0.0 version yet.

@adnan-ashraf
Copy link
Contributor Author

adnan-ashraf commented Aug 8, 2024

I've added the unit tests, please review the changes.

I was able to locally run the tests successfully only when I changed 0UL to 0ULL in GENMASK definition, or by changing BITS_PER_LONG to 32 in project.yml file.

@adnan-ashraf
Copy link
Contributor Author

I've fixed the GENMASK definition making it independent of BITS_PER_LONG, and also fixed some other issues. In addition, I've also improved the tests code. Please review the changes and provide any feedback, thanks!

@adnan-ashraf adnan-ashraf requested a review from Letme August 8, 2024 19:40
@Letme
Copy link
Member

Letme commented Aug 9, 2024

Can you also run stylechecker, as it seems uncrusitfy complains about something?

Copy link
Member

@Letme Letme left a comment

Choose a reason for hiding this comment

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

Thanks for the complete contribution. I do not have the environment ready to do a complete temperature range check, I will rely on your and my manual tests. Once I fix ceedling with released gem also the CI will maintain the quality.
I think stylechecker complains about a line length, but that should be a quick run fix and then we merge.

inc/mlx90632.h Outdated Show resolved Hide resolved
@adnan-ashraf adnan-ashraf requested a review from Letme August 9, 2024 10:55
@adnan-ashraf
Copy link
Contributor Author

I've run uncrustify, so the stylechecker is working successfully now!

@adnan-ashraf
Copy link
Contributor Author

I'm sorry that stylechecker is complaining again. I used uncrustify latest version 0.79.0 and it locally ran successfully. The stylechecker workflow is using uncrustify version 0.75.1. This could be the reason of failure.

@adnan-ashraf
Copy link
Contributor Author

I've updated the stylechecker to use the latest version of uncrustify. I think, it shouldn't complain now. Thanks!

@Letme Letme merged commit d441b78 into melexis:master Aug 9, 2024
10 of 19 checks passed
@Letme
Copy link
Member

Letme commented Aug 9, 2024

Great. Newer uncrustify is also one less worry for me to update. Thanks for the initiative.

@adnan-ashraf
Copy link
Contributor Author

adnan-ashraf commented Aug 9, 2024

Glad to hear, happy to contribute. Thanks for your time and feedback!

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.

Blocking operation
3 participants