search

arduino-libraries / Keyboard

GNU Lesser General Public License v3.0
229 stars 160 forks source link

CI: do not compile examples for Mbed boards #56

Closed edgar-bonet closed 2 years ago

edgar-bonet commented 2 years ago

Since the merge of pull request #46, the “Compile Examples” GitHub Actions workflow has systematically failed:

All the failures are caused by this error:

In file included from /[...]/examples/Serial/Serial.ino:22:0:
/[...]/src/Keyboard.h:25:10: fatal error: HID.h: No such file or directory
 #include "HID.h"
          ^~~~~~~

The underlying reason is that this library relies on the HID library, which does not support the mbed_nano and mbed_portenta platforms.

This pull request removes those boards from the list of boards this library is to be tested for.

github-actions[bot] commented 2 years ago

Memory usage change @ 7b421973479f8d766e874ee4d3a812e0ded8db22

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|examples/Serial
flash|%|examples/Serial
RAM for global variables|% -|-|-|-|- arduino:avr:leonardo|0|0.0|0|0.0 arduino:sam:arduino_due_x_dbg|0|0.0|N/A|N/A arduino:samd:mkrzero|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Serial
flash,%,examples/Serial
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0 ```
per1234 commented 2 years ago

Thanks @edgar-bonet!

The reason why I added compilation for these boards to the CI workflow is because I feel that the user will expect the library to support them: https://github.com/arduino-libraries/Keyboard/blob/644114924e2515caa6327a0776e2b57762d780e8/library.properties#L5

So it seems to me that there is one of two defects with the library:

OR

The purpose of the workflow is to identify defects. So if there is a defect then it is correct for the workflow run to fail and it would be better to resolve the defect rather than reconfiguring the workflow to ignore it.

There has been a slight improvement to the situation recently in that we finally have examples for the USBHID library, but there still is no real documentation for the library.

I also think that consideration should be given to the portability which has been a hallmark of the Arduino project. I should be able to use the same keyboard emulation sketch on any Arduino board with USB capability.

I do understand that the failure of this workflow causes a bad experience for the contributor.

edgar-bonet commented 2 years ago

Hi @per1234!

Allows an Arduino board with USB capabilities to act as a Keyboard.

This seems to me like an erroneous description. This sentence was never meant to imply that the library supports the Mbed-based boards: it was written back in 2015, well before these boards came out. When the boards became available, the sentence became erroneous, and nobody though of fixing it.

The purpose of the workflow is to identify defects. So if there is a defect then it is correct for the workflow run to fail

I do not think this is the correct way to use CI tests. The purpose of CI is to detect new defects:

If the CI systematically fails, maintainers get trained to ignore the CI results, and CI becomes useless. If a defect has been there from the inception of the project, the proper way to document it is to open an issue, not to make every single CI run fail.

Note that introducing a failing test to a CI pipeline is fine in a TDD approach, but such a test that is expected to fail should be pushed to a feature branch, right before the commit that fixes it. Making CI fail on master on purpose seems kind of wicked.

There has been a slight improvement to the situation recently in that we finally have examples for the USBHID library

Interesting. Thanks for the link! I see that it defines a USBKeyboard class that does the same as Keyboard, but unfortunately it is an independent implementation, which means duplicate effort.

I should be able to use the same keyboard emulation sketch on any Arduino board with USB capability.

Agree: that would be great! But I do not think there is an easy fix: for using this library on those boards, we would need an implementation of the HID library for Mbed.

I do understand that the failure of this workflow causes a bad experience for the contributor.

And for the maintainers that have to sort out failures into spurious warnings / real problems. That is, until they stop caring and just ignore the workflow failures.

aentinger commented 2 years ago

Thank you to both @edgar-bonet and @per1234 for bringing forth your arguments. I tend to side with @edgar-bonet and I believe @per1234 is doing the same, he is just polite (or smart) enough to try and come up with an explanation that makes half-way sense without going into Arduino internal issues. I'm not that smart so I'll just say that unfortunately this is a systematic issue stemming from the internal prioritisation of tasks within Arduino.

@edgar-bonet Can you please add another commit limiting the library only to those architectures, which are actually supported? This way CI will conform to the supported architecture list and vice-versa. I believe this to be a suitable compromise until we can eventually also support mbed-based-boards.

edgar-bonet commented 2 years ago

@aentinger: OK, I just pushed that. I listed only the architectures that are build-tested by the workflow: avr, samd, sam.

I hope I am not missing something, as I am not aware of the full Arduino ecosystem (I'm kind of an AVR enthusiast). The copy of the HID library I have on my desktop says “architectures=avr”, so I assume this library has per-architecture implementations. The list of library architecture variants is quite long, and for most of them I cannot tell whether they provide a suitable implementation.

aentinger commented 2 years ago

Thank you @edgar-bonet :bow: @per1234 What do you think? Does the new list conform to the CI target list?

github-actions[bot] commented 2 years ago

Memory usage change @ b1bf8bef796b1349e15568aafb978faac89cd450

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|examples/Serial
flash|%|examples/Serial
RAM for global variables|% -|-|-|-|- arduino:avr:leonardo|0|0.0|0|0.0 arduino:sam:arduino_due_x_dbg|0|0.0|N/A|N/A arduino:samd:mkrzero|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Serial
flash,%,examples/Serial
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0 ```