-
Notifications
You must be signed in to change notification settings - Fork 66
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
Custom devices new #258
Custom devices new #258
Conversation
Firmware for this pull request: |
Firmware for this pull request: |
@neilenns @GioCC may I ask you to have also a look on this PR? @neilenns would be great to check espacially the changes in the automatic build process: @GioCC I used the code from your PR for the TM1637 implementation, but deleted all MAX7219 related code to have another example for a custom device. If this is not OK for you, I will delete it or change to another lib. Or you try to setup this example as a custom device ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more explanations of the code changes
@@ -43,5 +49,5 @@ jobs: | |||
with: | |||
name: firmware | |||
path: | | |||
.pio/build/**/mobiflight*.hex | |||
.pio/build/**/mobiflight*.uf2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to the environment within platformio.ini to differ between plane mobilflight firmwares and supporter/user firmwares in the name
|
||
- name: Release | ||
uses: softprops/action-gh-release@v1 | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
with: | ||
files: | | ||
.pio/build/**/mobiflight*.hex | ||
.pio/build/**/mobiflight*.uf2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy all .hex and .uf2 files to the zip folder, not only these with mobiflight*
@@ -4,3 +4,4 @@ | |||
.vscode/launch.json | |||
.vscode/ipch | |||
.vscode/arduino.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the folder CustomDevice gets cloned during building. To have no changes shown this folder is excluded from synchronization
#define STEPPER_ACCEL 800 | ||
#ifndef MAX_CUSTOM_DEVICES | ||
#define MAX_CUSTOM_DEVICES 5 | ||
#endif | ||
|
||
#ifndef MOBIFLIGHT_TYPE | ||
#define MOBIFLIGHT_TYPE "MobiFlight RaspiPico" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all boards the custom devices is added, but excluded in build process for the existing environments
get_CustomDevices.py
Outdated
env.Execute("git clone --depth 100 https://github.com/elral/MobiFlight-CustomDevices $PROJECT_DIR/CustomDevices") | ||
else: | ||
print ("Checking for Mobiflight-CustomDevices repo updates ... ") | ||
env.Execute("git --work-tree=$PROJECT_DIR\CustomDevices --git-dir=$PROJECT_DIR\CustomDevices\.git pull origin main --depth 100") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file gets executed on each build from an environment. It clones the repo for custom devices or checks for updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this script needed? The repo already gets cloned by the changes made to the GitHub workflow scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For local builds.
But hmhm, maybe it's sufficient to provide the command to clone or update the repo. Or leave this script but execute it from the command line. Maybe this is better as new devices get compiled on the first run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, turns out I asked the same question (in a complementary way) before reading this...
The way I see it, I'm inclined to say that the cloning here should remain - for local builds it has to be there somewhere, For automated builds it can be in the script if this instance isn't applicable to that case for whatever reason (which is my question elsewhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this in the car this morning and this doesn't belong in the repo. I don't want a default script that automatically pulls from my custom device repo every time I do a build. That's ripe for issues with merge conflicts (someone else checked something in), etc. We also shouldn't have two different ways of cloning things in the release build process: the github action is the way to clone git repos in github actions, not git commands on the command line.
For local builds the developer can do a "git clone" once to set the custom device repo up, then a git pull whenever they deem it necessary to refresh the local copy. If people want to do some level of automation for their own local development process they can add that themselves to their fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also as it stands now this is causing an error in the automated build process:
git --work-tree=/home/runner/work/MobiFlight-FirmwareSource/MobiFlight-FirmwareSource\CustomDevices --git-dir=/home/runner/work/MobiFlight-FirmwareSource/MobiFlight-FirmwareSource\CustomDevices\.git pull origin main --depth 100
fatal: not a git repository: '/home/runner/work/MobiFlight-FirmwareSource/MobiFlight-FirmwareSourceCustomDevices.git'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was due to using backslashes in the path.
${env.extra_scripts} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for all boards the environments are added with mobiflight
to get the existent firmware file name (see above)
#if MF_CUSTOMDEVICE_SUPPORT == 1 | ||
cmdMessenger.attach(kSetCustomDevice, CustomDevice::OnSet); | ||
#endif | ||
|
||
#ifdef DEBUG2CMDMESSENGER | ||
cmdMessenger.sendCmd(kDebug, F("Attached callbacks")); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new callback for custom devices
// cmdMessenger.sendCmd(kDebug, F("CustomDevice loaded")); | ||
break; | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new case for supporting custom devices. All other changes should be reformatting
setLastCommandMillis(); | ||
} | ||
|
||
} // end of namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New functions for all custom devices.
No changes for new custom devices are required as all must use these functions.
CustomDevice::update(); | ||
#endif | ||
#endif | ||
|
||
// lcds, outputs, outputshifters, segments do not need update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defines within the platformio.ini for custom devices can enable these update() functions. As this is defined in the customer platformio.ini no changes are requires for new ones
WOW, @elral, that's a really impressive work! Congratulations are in order! |
One small thing I noticed: in all_custom_devices_mega.board.json in the other repo:
|
Thanks a lot, took me all the last weeks.
Great, and I see it also to have a native support.
Perfect, great finding. Will change it. The code within MFCustomDevice.cpp isn't optimized, just copied from the other files. I have done it in this way to show what has to be done for setting it up. Two times Thanks very much! |
I have written a few notes for myself while starting to look at the examples in the CustomDevices repository; I though I'd attach it here in case it contains something useful to add to the documentation (or not). And again... very clever! |
Oh, I forgot to mention that espacially the TM1637 is good example to have the so called configParameter. This one is already implemented in the firmware, but mot in the connector. The number of digits could be set into this configParameter. This would avoid the use of two device.json files and having only one device type in the code. Additionally the decimal point could be set, so no additional message to set this would be required. |
Perfect! It's exactly what the intention is, but much better described (than I could do ;) ) I am impressed how fast you got everything.
This would be great. The only thing I have to think about is where to copy your comments exactly. Another way would be to transfer the description soon to the Mobiflight WiKi. Then you could directly add your improvements there. /edit: Forgot to mention, everything is an outcome of mutliple and long discussions with @DocMoebiuz , not only my work |
.github/workflows/ci.yml
Outdated
- name: Checkout custom devices | ||
uses: actions/checkout@v3 | ||
with: | ||
repository: elral/MobiFlight-CustomDevices | ||
path: CustomDevices | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we'll never have custom devices in default MF firmware should we have this commented out in the repo? Then tell people building firmware with a custom device where to go to uncomment it for their own builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we not provide "some" custom devices which are officially reviewed and released?
And how do get users a firmware file if they uncomment this with the correct FW Version? I am doing it always by changing the get_version.py script ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about this. The script has Platform.IO run to to build the firmware. Doesn't Platform.IO itself clone the CustomDevices repo? What's the reason why this also has to be done in the script? Asking because I'm not that experienced in this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we have actual custom devices that ship with Mobiflight firmware directly we shouldn't be running a pull for them in the build.
When the day comes that we do decide to ship custom devices with the core firmware that PR can enable the pulling of the custom devices repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh never mind, I see there's custom devices coming already just the repo needs to be moved over. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about this. The script has Platform.IO run to to build the firmware. Doesn't Platform.IO itself clone the CustomDevices repo? What's the reason why this also has to be done in the script? Asking because I'm not that experienced in this.
Difficult for me to explain.
If you have a plain Installation, the folder /CustomDevice does not exist. Therefore the files listed in platformio.ini are not found and these environments get not into the list of environments to be compiled. On the next executing of the platformio.ini these files are available on they are getting into the list of environments to be compiled. As the automatic build process has always not the CustomDevice folder and pio run is only executed once, the custom device repo has to be cloned before.
Or what just comes in mind, a pio run - e mega and next a pio run could be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember if I asked this yet, but are these custom devices from the repo that has to move over intended to ship as part of the core Mobiflight firmware? They're just added using the custom device method instead of being coded as new specific devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are fully functional devices. From my side all can be moved to the new Mobiflight repo. And from my side all could be shipped with the main firmware.
But in the end it is the decision of @DocMoebiuz which one will be moved/shipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DocMoebiuz If the intention is to ship these as part of the core firmware they should be checked in to the firmware repo, not some external repo and then built in. If the intention isn't to ship them then these build steps should be disabled in the official firmware repo with guidance given to 3rd parties about how to enable them for their own builds.
Aside from complicating the build and local development process we lose valuable git benefits by storing them somewhere else:
- There's no easy way to know which iteration of the devices were built into a version of the firmware. The commit hash won't tell us and the source won't be included in the .zip attached to each release.
- Any changes made to those devices won't be included in the automatically generated release notes. We'll need to remember to manually add them every time.
- It'll be yet another repo for us to have issue tracking. We'll have to remember to look over there for issues related to code that directly ships in the firmware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DocMoebiuz If the intention is to ship these as part of the core firmware they should be checked in to the firmware repo, not some external repo and then built in. If the intention isn't to ship them then these build steps should be disabled in the official firmware repo with guidance given to 3rd parties about how to enable them for their own builds.
Aside from complicating the build and local development process we lose valuable git benefits by storing them somewhere else:
- There's no easy way to know which iteration of the devices were built into a version of the firmware. The commit hash won't tell us and the source won't be included in the .zip attached to each release.
- Any changes made to those devices won't be included in the automatically generated release notes. We'll need to remember to manually add them every time.
- It'll be yet another repo for us to have issue tracking. We'll have to remember to look over there for issues related to code that directly ships in the firmware.
@neilenns thanks for your valuable feedback! In general, I want to create an opportunity for the community to extend MobiFlight at their on pace and without having me (or us) become the bottleneck. I don't want to be part of the custom device life cycle. At the moment I don't see any custom device become built-in in the official mobiflight firmware. I envision and hope that people come up with their custom device implementations and at the moment I expect them to compile a specific firmware versions, hence board type, for it - or leave it up to the individual user to compile a custom firmware on their own.
@elral has experimented with including different custom devices as repository references. It reminds me of npm or pip where you install a library. There are still some lines of code that have to be adapted to use those custom devices and expose them correctly. But i think for the first release that is perfectly fine and makes it very easy to get started. our assumption is that a user who develops custom firmware will find the process easy enough.
So to specifically answer your questions:
- There's no easy way to know which iteration of the devices were built into a version of the firmware. The commit hash won't tell us and the source won't be included in the .zip attached to each release.
... that's OK. the custom firmware can have it's own version. It will have a base firmware version of the official mobiflight firmware and a combination of custom device versions... but it is the responsiblity of the custom firmware creator to maintain this up-to-date in case there are improved versions available.
- Any changes made to those devices won't be included in the automatically generated release notes. We'll need to remember to manually add them every time.
... that's OK, I see these devices outside of the official releases anyways. If we have an official MobiFlight Community Repository with curated custom devices (like npm registry) then we can have release notes in that repository.
- It'll be yet another repo for us to have issue tracking. We'll have to remember to look over there for issues related to code that directly ships in the firmware.
... that's OK, this is the responsibility of the respective device developer or repository maintainer. For the beforementioned official MobiFlight Custom Device repository I am sure that we can find a feasible solution.
And to say it one more time: I see a big advantage for the community to decouple the generic devices from the official MobiFlight Core Firmware. We keep it this way on purpose so that the community can move on its own pace. We can still think of some incubator process later which might help to bring popular and stable custom devices to the official MobiFlight Community Repository.
Maybe we can find a cool way to automatically create custom firmwares in the future, where the user has the ability to select from a list of available devices the ones they like to have in their firmware. I am convinced that this first iteration provides already enough options to unlock a lot of creativity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked a couple of clarification questions
One more comment from my side. @DocMoebiuz it would be good if we could add pins to the device.json file which the custom device uses. These pins should get be marked as used in the connector. E.g. by using HW spi the required pins are fixed (at least for the AVR's). These pins would be in the device.json file and not he deleted in the board.json.file. For firmwares with multiple devices a user could define some supported devices but not necessarily the custom device which has the HW SPI. In this case these pins should still be available. /edit: the example of the GNC255 uses HW SPI and has for now a remark in the Readme file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to ask a question
Output::Add(params[0]); | ||
copy_success = readEndCommandFromEEPROM(&addreeprom); // check EEPROM until end of name | ||
copy_success = readEndCommandFromEEPROM(&addreeprom, ':'); // check EEPROM until end of name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the addition of a ":" here a breaking change with old firmware? What happens if old firmware connects to new mobiflight connector after this is released?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's backwards compatible. Just a combination of two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok phew!
src/Config.cpp
Outdated
uint16_t length = MFeeprom.get_length(); | ||
char temp = 0; | ||
uint16_t adrPin = 0; | ||
uint16_t adrType = 0; | ||
uint16_t adrConfig = 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all causing unused variable warnings in the build:
src/Config.cpp:250:14: warning: unused variable 'length' [-Wunused-variable]
uint16_t length = MFeeprom.get_length();
^~~~~~
src/Config.cpp:251:14: warning: unused variable 'temp' [-Wunused-variable]
char temp = 0;
^~~~
src/Config.cpp:252:14: warning: unused variable 'adrPin' [-Wunused-variable]
uint16_t adrPin = 0;
^~~~~~
src/Config.cpp:253:14: warning: unused variable 'adrType' [-Wunused-variable]
uint16_t adrType = 0;
^~~~~~~
src/Config.cpp:254:14: warning: unused variable 'adrConfig' [-Wunused-variable]
uint16_t adrConfig = 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to check it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remember what I did ;)
Changed it.
platformio.ini
Outdated
@@ -15,6 +15,15 @@ | |||
; development use VSCode to change the target to a non-default | |||
; by clicking on the target name in the bottom status bar. | |||
[platformio] | |||
; include custom environments to be build | |||
extra_configs = | |||
./CustomDevices/_template/MyCustomDevice_platformio.ini |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template shouldn't be included, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will remove it. Also in the all devices environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -69,6 +72,9 @@ | |||
#ifndef MAX_DIGIN_MUX | |||
#define MAX_DIGIN_MUX 4 | |||
#endif | |||
#ifndef MAX_CUSTOM_DEVICES | |||
#define MAX_CUSTOM_DEVICES 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We screwed up when adding shifters because we did output first and just called them "shifter". Then when I came and added input shifters they got called "inputshifter".
Let's not make the same mistake again with custom devices and get it right the first time. Call them CUSTOM_OUTPUT_DEVICES and make sure all the relevant files are called "CustomOutputDevice".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second that, good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed some time to think about.
I am still not convinced that it's required to rename it (in contrast to the shifters). The user can setup a custom device as input device, output device or both (how to name in this case). In the end it is a custom devices and the code defines what it is. From the firmware side mainly only a handler is missing which gets called if a defined action is ececuted. For the connector it's function (input and/or output) could be marked in the accordingly device.json file.
Firmware for this pull request: |
Firmware for this pull request: |
Firmware for this pull request: |
Firmware for this pull request: |
Firmware for this pull request: |
Firmware for this pull request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It is time to merge this.
@@ -33,6 +33,12 @@ jobs: | |||
python -m pip install --upgrade pip | |||
pip install --upgrade platformio | |||
|
|||
- name: Checkout custom devices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree... we should not do this and use the platform.io feature for including additional dependencies
This PR adds support for custom devices. This makes it a lot of easier to implement own devices.
A defined interface for the data exchange is introduced.
There was already PR #214 which was closed as major changes are implemented in this new branch.
The code for custom devices will have an own repo, for now it's https://github.com/elral/MobiFlight-CustomDevices which must be transferred to Mobiflight when this PR gets merged. Additionally the script
get_CustomDevices.py
and the workflowsci.yml
/release.yml
must be adapted in this case. They point for now to my repo to get the PR compiled on github.On first executing of
pio run [-e environment]
the repo for custom devices will be cloned into the folder./CustomDevices
. On all next executing ofpio run [-e xyz]
this folder gets updated and all available custom devices gets compiled.All the required code for a custom device must be in one folder. How to setup this code is explained in https://github.com/elral/MobiFlight-Connector/wiki/How-to-set-up-a-custom-device (which must also be transferred to the Mobiflight WiKi) and in a template under
./CustomDevices/_template/
.To get it automatically compiled during a
pio run
theplatformio.ini
must be adapted with one additional line which points to an .ini file whithin the device folder.Some examples are already included.
For the naming convention the names of the standard environments are changed. The
mobiflight
is now part of this environment and not anymore added by theget_version.py
script. This enables to have firmware file names starting not only with mobiflight but also with a different name (most likely [company]name of the user/company).The boards.h files are added for using custom devices. The config.cpp is added to read a config with custom devices. The commandmessenger.cpp is added with new functions to call the custom device functions. A new folder
/src/CustomDevices
is added where the basic functions to support custom devices are included. These files are always the same and must not be changed for a new custom device.