search

melexis / mlx90632-library

MLX90632 library for the Melexis 90632 Infra Red temperature sensor.
Apache License 2.0
42 stars 15 forks source link

Non-blocking functions #50

Closed adnan-ashraf closed 2 weeks ago

adnan-ashraf commented 3 months ago

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

adnan-ashraf commented 2 weeks ago

Can someone please review this pull request? @Letme

Letme commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

Thanks for taking the time to review!

Appreciate the feedback! Your suggestions have been added.

Sure, I'll write the tests.

yogeshiggalore commented 2 weeks ago

when this will get merged..?

Letme commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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!

Letme commented 2 weeks ago

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

adnan-ashraf commented 2 weeks ago

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

adnan-ashraf commented 2 weeks ago

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 commented 2 weeks ago

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

Letme commented 2 weeks ago

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

adnan-ashraf commented 2 weeks ago

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