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

add Arduino build + English manual #62

Merged
merged 4 commits into from
Feb 8, 2025
Merged

Conversation

RobTillaart
Copy link
Contributor

English manual by google translate and quickly reviewed by me. Needs verification.

@RobTillaart RobTillaart mentioned this pull request Jun 23, 2024
@RobTillaart
Copy link
Contributor Author

@RobTillaart
Copy link
Contributor Author

@amotl
When merging best use the SQUASH option so all my commits will merge into one.

@amotl
Copy link
Member

amotl commented Jun 23, 2024

Dear Rob,

wow, that was quick, and your patch is really extensive. Thank you so much!

With kind regards,
Andreas.

@RobTillaart
Copy link
Contributor Author

@amotl
You're welcome!
The English handbook looks OK too (imho), it is processed without problems by readthedocs.yml.

Comment on lines +1 to +14
platforms:
rpipico:
board: rp2040:rp2040:rpipico
package: rp2040:rp2040
gcc:
features:
defines:
- ARDUINO_ARCH_RP2040
warnings:
flags:

packages:
rp2040:rp2040:
url: https://github.com/earlephilhower/arduino-pico/releases/download/global/package_rp2040_index.json
Copy link
Member

Choose a reason for hiding this comment

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

I see that esp32 is selected down below. In this spirit, I am wondering why any references to the Pico are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These rpipico lines are not needed per se, however it would allow you to include it in a future test.
Pico is not supported directly in the core Arduino build.
This is what is needed to get the build working with an rpipico compiler.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

Comment on lines +28 to +31
libraries:
- U8g2
- HX711
- ESP32Servo
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to apply version pinning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of, default it uses the latest versions as far as I know.

Copy link
Member

Choose a reason for hiding this comment

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

All right, thank you. We would like to employ version pinning here, because newer libraries may not work well with HaniMandl in production.

I will look into this on another iteration: Maybe Arduino CI gained that feature in the meanwhile?

Comment on lines +15 to +17
- run: |
gem install arduino_ci
arduino_ci.rb
Copy link
Member

@amotl amotl Jun 23, 2024

Choose a reason for hiding this comment

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

We can probably read up on it on the documentation of Arduino-CI, but let us also ask right here: What does invoking arduino_ci.rb actually do in this context? Does it even "run" the sketch to completion, and how?

On the Arduino-CI docs, I can find:

[...] provides a system of mocks that allow fine-grained control over the hardware inputs, including the system's clock

We can't imagine some set of standard mocks can satisfy the requirements of HaniMandl easily. What about the display interface, for example?

Copy link
Member

Choose a reason for hiding this comment

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

We guess the current state "just compiles" the code, and verifies it on behalf of that outcome, just like platformio-ci.yaml is doing it, right?

Coming from there, do you have any suggestions how we could apply better unit testing to the codebase? I guess it would need to be tremendously modularized, in order to achieve that?

Apologies for our beginner's questions. Please do not spend too much time answering them, we are also writing them down for our own reference, in order to outline where we will need to level up on understanding and using Arduino-CI properly. The right way will be to do it hands-on, paired with reading relevant documentation sections. We just didn't get around doing it, yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does invoking arduino_ci.rb actually do in this context? Does it even "run" the sketch to completion, and how?

arduino_ci.rb is a ruby script that calls the command line Arduino compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't imagine some set of standard mocks can satisfy the requirements of HaniMandl easily. What about the display interface, for example?

Mocks are not too difficult, but takes quite some effort to implement, so I do not know if it is worth the effort.

Mocks are ideal for testing a part of a library, when you would create some core "hanimandl" library it might make sense to put the effort in. A way to do this is to split of typical functions in a separate hanimadl.h file and then you would be able to test these functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We guess the current state "just compiles" the code, and verifies it on behalf of that outcome, just like platformio-ci.yaml is doing it, right?

That is right, however it does compile the code under the Arduino IDE, not under the PlatformIO environment. As these are different it adds "robustness". The same is true for doing the LINT check, it does look at the code and even if it compiles it warns you of some risky code constructs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming from there, do you have any suggestions how we could apply better unit testing to the codebase? I guess it would need to be tremendously modularized, in order to achieve that?

Yes, think so.
As said before, the first step is to extract some typical hanimandl functions.

I looked quickly at the code and I see you have organized it in a way that keeps all things close to each other, however many functions do handle pins, do display work and changes some setting etc. A program of this size I would split of all display handling in one file, all the reading of buttons (pin IO) in one file, the handling of rotary encoders in one file.
Furthermore I would have a STATE variable that defines the modus operandi of the program.
Am I in state_setup, state_filling or state_error, etc.
So yes, quite some work.

Copy link
Member

Choose a reason for hiding this comment

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

I absolutely agree with this plan, and also agree it will be quite some work. Maybe a future version of HaniMandl will be refactored like this, I would really appreciate this kind of modularization of the code base.

Copy link
Member

@amotl amotl Jun 23, 2024

Choose a reason for hiding this comment

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

Is there a reason attached to this, that the code had to be duplicated into examples/hanimandl/hanimandl.ino?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, you elaborated about this details over at #18 (comment):

Note that I use the Arduino-CI mainly for libraries, and I used it for an application by adding an "examples" folder, in which the applications are copied as if they were examples from a library. See - https://github.com/RobTillaart/MultiSpeedI2CScanner

Maybe it is applicable to use a symlink for that purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason attached to this, that the code had to be duplicated into examples/hanimandl/hanimandl.ino?

As the Arduino build CI is originally developed for testing libraries, it is the trick to make it testing stand alone programs.

Maybe it is applicable to use a symlink for that purpose?

Good question, never tested,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it is applicable to use a symlink for that purpose?

The build-CI VM runs ubuntu, my develop environment is windows, so the symlink semantics might differ.
Needs some investigation.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks! I think I will still try to use a symlink here, and defer the investigation about Windows to a subsequent iteration. I hope we can find a solution that works on all platforms.

README.md Outdated
@@ -4,6 +4,8 @@
![Documentation](https://readthedocs.org/projects/hanimandl/badge/)
![Version](https://img.shields.io/github/v/tag/hiveeyes/hanimandl.svg)

[![Arduino CI](https://github.comhiveeyes/hanimandl/workflows/Arduino%20CI/badge.svg)](https://github.com/marketplace/actions/arduino_ci)
Copy link
Member

@amotl amotl Jun 23, 2024

Choose a reason for hiding this comment

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

Just spotted a one-character typo.

Suggested change
[![Arduino CI](https://github.comhiveeyes/hanimandl/workflows/Arduino%20CI/badge.svg)](https://github.com/marketplace/actions/arduino_ci)
[![Arduino CI](https://github.com/hiveeyes/hanimandl/workflows/Arduino%20CI/badge.svg)](https://github.com/marketplace/actions/arduino_ci)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix that one right away.

@amotl
Copy link
Member

amotl commented Jun 23, 2024

I just ran through a quick review, yielding some comments shared above, and will be happy to try addressing all details on my own behalf before merging. When doing it, I will also squash your commits as suggested. Thanks again!

@RobTillaart
Copy link
Contributor Author

@amotl

Two things you could start using:

  • label the open issues (bug, enhancement, question, etc) to prioritize them.
  • use a const char version[12] = "X.Y.Z" in the application, to be displayed on the opening screen
    • use the GitHub Releases to formalize this version number.
    • people can easily refer to the version as they see it at startup.

@RobTillaart
Copy link
Contributor Author

You might want to have this badge in the readme too?

GitHub issues

[![GitHub issues](https://img.shields.io/github/issues/hiveeyes/hanimandl.svg)](https://github.com/hiveeyes/hanimandl/issues)

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thanks a stack for your work on this. Apologies for the delay about only now coming back to this. I will now refresh your patch, squash the commits, add a few adjustments, and finally merge it.

Comment on lines +1 to +14
platforms:
rpipico:
board: rp2040:rp2040:rpipico
package: rp2040:rp2040
gcc:
features:
defines:
- ARDUINO_ARCH_RP2040
warnings:
flags:

packages:
rp2040:rp2040:
url: https://github.com/earlephilhower/arduino-pico/releases/download/global/package_rp2040_index.json
Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

Comment on lines +28 to +31
libraries:
- U8g2
- HX711
- ESP32Servo
Copy link
Member

Choose a reason for hiding this comment

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

All right, thank you. We would like to employ version pinning here, because newer libraries may not work well with HaniMandl in production.

I will look into this on another iteration: Maybe Arduino CI gained that feature in the meanwhile?

Comment on lines +15 to +17
- run: |
gem install arduino_ci
arduino_ci.rb
Copy link
Member

Choose a reason for hiding this comment

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

I absolutely agree with this plan, and also agree it will be quite some work. Maybe a future version of HaniMandl will be refactored like this, I would really appreciate this kind of modularization of the code base.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks! I think I will still try to use a symlink here, and defer the investigation about Windows to a subsequent iteration. I hope we can find a solution that works on all platforms.

@amotl amotl merged commit 81b2c37 into hiveeyes:main Feb 8, 2025
5 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.

2 participants