WifWaf / MH-Z19

For Arduino Boards (&ESP32). Additional Examples/Commands., Hardware/Software Serial
GNU Lesser General Public License v3.0
195 stars 39 forks source link

Some fixes #51

Closed ArminJo closed 1 year ago

ArminJo commented 1 year ago

Hi, Thanks for the library I plan to use for my project!

I removed trailing blanks, fixed some typos, adjusted some types and documentation and fixed some errors in examples. I also included a simple CI with Github Actions and enabled the examples for compiling on an ESP32.

May I also suggest to enable discussions in Settings menu and transfer all open issues to discussions. And you can disable/uncheck unused packages and environment here: 2022-10-12 22_00_05-ArminJo_Arduino-BlueDisplay_ Arduino library for the BlueDisplay App  The App co

WifWaf commented 1 year ago

Hey,

I'm honestly not familiar with .yml configure and how they work during compilation, so will have a quick read-up in a few days and look over the changes (unwell with covid at the moment, so not much use).

Cheers for taking the time to put this together - it should be fine at a quick glance.

ArminJo commented 1 year ago

Results of the CI can be seen here https://github.com/ArminJo/MH-Z19/actions

ArminJo commented 1 year ago

Everything OK?

WifWaf commented 1 year ago

Yes, sorry for not getting back to you on this.

I had a read over the changes and the corrections are much appreciated. Spelling is not my forte... I've added a few comments which need to be looked at - I assume you can see these? (I'm not experienced with the pull request system). *OK - I can see you can!

In regards to the basic example changes, generally, I try to keep the basic example as simple as possible to avoid any confusion as Arduino is a common entry point to programming. Preprocessor directives are a bit more advanced, so for the ESP32 setup, it might be best to comment out the relevant code with a short description (e.g. "for ESP32 users").

Let me know your thoughts.

*As a side note, I'm not entirely sure how the GitHub actions/workflow works - is it necessary to include the .yml file?

Cheers,

ArminJo commented 1 year ago

Hi,

the preprocessor directives enables the ESP32 user to compile the examples without errors on the first run (and often, but not in this case) to pass the Github actions. Without it, the user first has to change comments before succesfull running, which is not user friendly in my opinion. But YES you are right it increases the complexity of the examples! But I guess the novice user just overreads this preprocessor directives (and other things :-() and compile it and then modify it. In my ecxperience there are only a few, who try to understand all parts of the code. But additional comments are a good idea 😀 . For my libraries, I decided to have at least one very simple example, and others are more sophisticated, to show users how to program a bit more professional.

And yes you have to include the *.yml file to activate the github actions.

WifWaf commented 1 year ago

Looks greats. I've merged the changes.

There is a more lightweight version of the library on the PR_2.0.0 branch, although it's slightly out of date. I've been planning (*tm) on moving it over gradually to the master branch.

I'll make sure the changes are moved to this branch too.

ArminJo commented 1 year ago

Thanks! 👍 I also refactored the library to make the processing more transparent and added some functions like printCommand() and debug output. But I am not finished yet. Best regards Armin