Skip to content

Conversation

@raphaelcoeffic
Copy link
Member

@raphaelcoeffic raphaelcoeffic commented Jun 5, 2021

This PR continues the MAVLink integration of #122.

TODOs

Cosmetics / Code structure

  • Remove Olli's markers (// OW / // OWEND).
  • Use the standard GPLv2 boiler plate instead of Olli's copyright notice.
  • Remove useless USB_MAVLINK_MODE and USB serial description specific to MAVlink (USB serial is just "USB serial").
  • Remove MAVLink includes from radio/src/opentx.h and add to appropriate files that need it.
  • Move unrelated function in radio/src/lua/api_mavlink.cpp to somewhere else (does anyone really need this?).
  • Clean-up changes datastructs.hand remove useless definitions.

UI

  • Move MAVLink telemetry setup items from "Model Setup" to "Model / Telemetry".

Fix breaking changes

  • Allow for using CLI and or DEBUG, even if MAVLink support is enabled.
  • Fix INTERNAL_GPS when compiled with/without TELEMETRY_MAVLINK.
  • Check why g_eeGeneral.mavlinkExternal == 1 should disable external module functionality (probably not needed).
  • Fix / remove changes to radio/src/targets/horus/hal.h (see #if (defined(RADIO_T16) || defined(RADIO_T18)) && defined(AUX_SERIAL)).

Code re-use

  • Use existing Serial Rx instead of mavlinkTelemUsbRxFifo & inspect other added FIFOs (check auxSerialTxFifo as well).
  • Check why uint16_t telemetryStreaming (normally uint8_t).

@olliw42
Copy link
Contributor

olliw42 commented Jun 5, 2021

After @olliw42 decided not to finish his PR

wau ...

For the record, I was closing the other PR because you were asking for several very serious changes to the code which I don't see can easily be accommodated in that code without breaking or stripping of crucial functionality, and it was you and not me who has set the conditions and didn't find the suggested solutions acceptable. And now you are just happy to go with the code ignoring your own comments .... and in addition promote misrepresented intent.

sad to see this behavior

👎

@raphaelcoeffic
Copy link
Member Author

For the record, I was closing the other PR because you were asking for several very serious changes to the code which I don't see can easily be accommodated in that code without breaking or stripping of crucial functionality, and it was you and not me who has set the conditions and didn't find the suggested solutions acceptable. And now you are just happy to go with the code ignoring your own comments .... and in addition promote misrepresented intent.

A long text to explain that you won't finish the job, which is basically the bottom line.

@raphaelcoeffic
Copy link
Member Author

And now you are just happy to go with the code ignoring your own comments

You missed the point: this PR is not finished, take a look at the TODO-list.

@rotorman
Copy link
Member

rotorman commented Jun 5, 2021

Dear @olliw42, I was surprised and at the same time devastated to learn that you closed PR #122, so close to being merged. You wrote that a revised attempt should go into new PR, well let's take a chance to finish MAVLink integration to ETX master here!
OTX/ETX community has long waited for native MAVLink in ETX/OTX (many attempts in OTX GH... also from many years ago already).
As @raphaelcoeffic has now himself re-created this PR, this is a clear indication that MAVLink support is much desired in EdgeTX ;)))

Reading the comments at the end of #122, they do make a lot of sense to me and in short list two major issues to be tackled:

  • use existing USB CDC/VCP Serial, instead of custom serial for MAVLink
  • allow CLI & DEBUG to be used at the same time with MAVLink (on serial ports not used by MAVLink)

Olli, you have done absolutely marvelous work with MAVLink integration, this is your baby! It would be awesome if you could consider joining with ideas allowing the last remaining points to be solved.

@rotorman
Copy link
Member

rotorman commented Jun 5, 2021

The option to use JR-Bay for MAVLink is possible only with a custom hw, pictured here:
https://www.rcgroups.com/forums/showpost.php?p=46948695&postcount=711
This might be something not initially usable for users, as the schematic mod has not been published by @olliw42 yet. Thus it might be an idea first to comment out this functionality, as this would possibly also fix the JR-Bay power issue listed here:
#36 (comment)

Update: The schematic and src. of the required STM32F1 µC are now available at Olli's GH page:
https://github.com/olliw42/otxtelemetry/tree/master/SPortBridge

@olliw42
Copy link
Contributor

olliw42 commented Jun 7, 2021

Use the standard GPLv2 boiler plate instead of Olli's copyright notice.

just for the record, changing copyright without permission by the copyright holders would constitute copyright infringment

@raphaelcoeffic
Copy link
Member Author

Use the standard GPLv2 boiler plate instead of Olli's copyright notice.

just for the record, changing copyright without permission by the copyright holders would constitute copyright infringment

You are still the copyright holder, that will not change. However, the license is still GPLv2.

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

Tested 7a19a3a on RM TX16S. AUX1 - MAVLink (ArduPilot via DragonLink), AUX2 - GPS, top USB - Mission Planner (MAVLink via virtual serial port).
MAVLink data flows as expected, internal GPS functions.

Instead of using "CDC" (official USB class name for virtual serial port, instead of "Debug"), naming of USB connection mode in the selection dialog might be simplified:

  • USB Joystick (HID) -> USB Joystick
  • USB Storage (SD) -> USB Storage
  • USB Serial (Debug) -> USB Serial

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

I gave further thought to my last post above - if DEBUG and/or CLI are built in, then we do need to distinguish which one the user wants to activate for USB, if he selects serial: Debug/CLI or MAVLink.
Hmm... any ideas where to do it? For Serial port, there is a selection under SYSTEM ->Hardware -> Serial port, but not for USB (yet).

@raphaelcoeffic
Copy link
Member Author

Instead of using "CDC" (official USB class name for virtual serial port, instead of "Debug"), naming of USB connection mode in the selection dialog might be simplified

Done!

@raphaelcoeffic
Copy link
Member Author

I gave further thought to my last post above - if DEBUG and/or CLI are built in, then we do need to distinguish which one the user wants to activate for USB, if he selects serial: Debug/CLI or MAVLink.

We should have a setting similar to what we have for the normal serial ports.

@olliw42
Copy link
Contributor

olliw42 commented Jun 8, 2021

Use the standard GPLv2 boiler plate instead of Olli's copyright notice.

just for the record, changing copyright without permission by the copyright holders would constitute copyright infringment

You are still the copyright holder, that will not change. However, the license is still GPLv2.

I repeat, changing copyright without permission by the copyright holders would constitute copyright infringement
like it or don't, but that's the rules
this is not my PR, so the rules apply
that's the facts

@raphaelcoeffic
Copy link
Member Author

Use the standard GPLv2 boiler plate instead of Olli's copyright notice.

just for the record, changing copyright without permission by the copyright holders would constitute copyright infringment

You are still the copyright holder, that will not change. However, the license is still GPLv2.

I repeat, changing copyright without permission by the copyright holders would constitute copyright infringement
like it or don't, but that's the rules
this is not my PR, so the rules apply
that's the facts

I don't understand: are you trying to publish something in a GPLv2 software without licensing that change with GPLv2? This does not work. If you cannot accept that, we will have to reject the code, as it is pre definition licensed under GPLv2 when it is added to the same software.

Or is the fact that I added "Copyright EdgeTX" that is your issue? Please explain further.

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

@olliw42 By looking at diff f7c0036 your copyright lines are still all there, only std. GPL v2 boiler, as OTX/ETX std., was added by @raphaelcoeffic.
Can you please point to where do you see an issue with this or how would you prefer the header to further list your main authorship?

@raphaelcoeffic
Copy link
Member Author

@olliw42 you might find this interesting: https://opensource.com/article/20/10/copyright-notices-open-source-software

"While these notices may appear important, their presence in source code today is largely a residue of the copyright law of the past. There was a time when failure to include a copyright notice in published material could result in complete loss of rights under US copyright law; that changed when the United States finally joined the many other countries that were already parties to the Berne Convention (US accession to the treaty came on November 16, 1988, and became effective in the US on March 1, 1989)."

@raphaelcoeffic
Copy link
Member Author

@olliw42 if that helps, I can assure you I will merge this with a proper merge commit, so that your contribution will stay visible, commit by commit.

@pfeerick
Copy link
Member

pfeerick commented Jun 8, 2021

I repeat, changing copyright without permission by the copyright holders would constitute copyright infringement
like it or don't, but that's the rules
this is not my PR, so the rules apply
that's the facts

"Changing copyright without permission by the copyright holders" is indeed copyright infringement. However, that has not happened here. You contributed code to a GPLv2 licensed project, hereby granting copyright to that project, and all derivative works. Your copyright to the code has not been modified, nor has the text indicating that been removed. The text indicating copyright has simply been reformatted to be consistent with rest of the source code for the project. Those are the facts, as documented by the commit logs.

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

Tested 04a91fb on RM TX16S, built with:
-DPCB=X10 -DPCBREV=TX16S -DDEFAULT_MODE=2 -DGVARS=YES -DPPM_UNIT=US -DHELI=NO -DLUA=YES -DINTERNAL_GPS=YES -DTELEMETRY_MAVLINK=YES -DCMAKE_BUILD_TYPE=Debug
AUX1 - MAVLink (ArduPilot via DragonLink), AUX2 - NMEA GPS, top USB-C - PC with Mission Planner.

By plugging in the top USB-C, now a popup with the following options comes up:

  • USB Joystick
  • USB Storage
  • USB Serial

MAVLink (still) works via AUX1, also via USB, internal GPS works. :)

Text in full screen (modified) Yaapu LUA script is a bit shifted. This is also visible in @yaapu 's PR #151 screenshot (all text basically, but see e.g. the HUD top center heading "0", or the green digits in HUD, where they clearly are not there where they should be):

@yaapu mentioned a 3 pixel offset in Discord: https://discord.com/channels/839849772864503828/842693696629899274/850753828382965811

Cannot build yet with added -DDEBUG=YES -DCLI=YES (expected, as the code still needs mods to allow this)

@olliw42
Copy link
Contributor

olliw42 commented Jun 8, 2021

come on, guys, pl resort back to common sense

However, that has not happened here. You contributed code to a GPLv2 licensed project

no, I don't. This is @raphaelcoeffic 's PR, not mine

hereby granting copyright to that project

ergo I don't

all you are granted therefore are the rules of os

Your copyright to the code has not been modified

it has been

so, in your opinion, you buy a CD of music of someone who has the (c), you stick your (c) in addition to it claiming (c) and argue that's ok since the other (c) was not modified ... so effectively anyone can claim (c) of any others works since all one needs to do is to stick one's own (c) onto it in addition to the previous (c) ... I assume that all can agree that this is obviously nonsense, I at least think this can't be so difficult to understand

since there seems to be confusion about what I'm talking about, I explictly state that I was not commenting on attribution, I am commenting on copyright

manoman

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

@olliw42 Can you please propose a solution, how this could be fixed to your liking, together with the std. EdgeTX boiler?

@olliw42
Copy link
Contributor

olliw42 commented Jun 8, 2021

@olliw42 Can you please propose a solution, how this could be fixed to your liking, together with the std. EdgeTX boiler?

that's impossible, that's the nature of it
you can't have it together with the std ETX boiler since it has (c)

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

@olliw42 Would you please propose an alternative boiler plate that is to your liking? I'm confident, we are able to work smth. out so that everyone will be happy at the end!

@olliw42
Copy link
Contributor

olliw42 commented Jun 8, 2021

Would you please propose an alternative boiler plate that is to your liking?

I could but I won't since this part is 100% up to your liking and not mine

I'm confident, we are able to work smth. out so that everyone will be happy at the end!

it's easy to achieve and it is surprising that it (again) is such a difficult process. Create anything you want as long as it does not change copyright, as easy as that.

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

We need something to discuss and/or agree on, so allow me to make a proposal, tackling the issue with double (c) lines you mentioned:

* (c) www.olliw.eu, OlliW, OlliW42
* 
* Part of EdgeTX project - https://github.com/EdgeTX/edgetx
* Based on code named
*   opentx - http://github.com/opentx/opentx
*   th9x - http://code.google.com/p/th9x
*   er9x - http://code.google.com/p/er9x
*   gruvin9x - http://code.google.com/p/gruvin9x
*
* License GPLv2: http://www.gnu.org/licenses/gpl-2.0.html
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

Would that be OK with you?

@olliw42
Copy link
Contributor

olliw42 commented Jun 8, 2021

it is not part of the EdgeTx project, it is part of the MAVLink for OpenTx project, and it has been taken over from there

what I find most projects to do in such cases is to keep the original header and add below something commenting (only) on additions/modifications to the code, or something like adapted to blabla or whatever, I don't have a specific example in mind but a bit google should give ideas

@rotorman
Copy link
Member

rotorman commented Jun 8, 2021

How about:

* (c) www.olliw.eu, OlliW, OlliW42
* 
* Based on EdgeTX pull request #122
* 
* EdgeTX project - https://github.com/EdgeTX/edgetx
* Based on code named
*   opentx - http://github.com/opentx/opentx
*   th9x - http://code.google.com/p/th9x
*   er9x - http://code.google.com/p/er9x
*   gruvin9x - http://code.google.com/p/gruvin9x
*
* License GPLv2: http://www.gnu.org/licenses/gpl-2.0.html
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

@brainbubblersbest
Copy link

brainbubblersbest commented Jun 9, 2021

Check why g_eeGeneral.mavlinkExternal == 1 should disable external module functionality (probably not needed).

Isnt that needed for the uart via s.port pin in external module bay? As from Olli planned
(He also developed a Platine Layout for This)
To make a serial Radio hf module possible?
Then this maybe Collides with tbs and r9 modules.
Again: maybe utilize the first and second hf bay pin in combination with express and externalaccessmod Radios would be better?
EG:
Set External HF Type - > Serial Radio Module
Or
Set External HF Type - > Crossfire w. MAV over Serial
(rc and crsf telemetry still over s. Port)

This would also solve the question for deactivating external modules. (Then its deactivated only if a specific Module isnt selected)

If course second external inversion would be needed for a Bay Serial Radio Module.

@raphaelcoeffic
Copy link
Member Author

@brainbubblersbest the plan was to include it once the hardware is available, which could give us some time to make better / more flexible serial port drivers.

@brainbubblersbest
Copy link

brainbubblersbest commented Jun 9, 2021

The Hardware for two wire Serial is already available.
1- All we need here is:
-a Serial Radio module
-two tbs inverters
-a express Radio or a Radio with the externalaccessmod.
(easy wiring to do on non frsky Radios)
-an old HF Module Case to mount everything inside.

2- Or a r9m 2019 hf module to modify and make use of the already included Inverters here.

3- In Addition: i ordered an hf Module Adapter for the Fullsize crossfire to figure out if the mav-link data here can easy linked out to pin 1 and 2. Just for tinkering a bit how complicated this would be.

rotorman added a commit to rotorman/edgetx that referenced this pull request Feb 6, 2022
The MAVLink specific settings are not yet implemented in YAML, otherwise tested on RM TX16S with DragonLink and ArduPilot.
Also for the added submodules, need to revert to using older versions from PR EdgeTX#150 and not their current heads.

Added MAVLink YAML elements.
rotorman added a commit to rotorman/edgetx that referenced this pull request Feb 6, 2022
The MAVLink specific settings are not yet implemented in YAML, otherwise tested on RM TX16S with DragonLink and ArduPilot.
Also for the added submodules, need to revert to using older versions from PR EdgeTX#150 and not their current heads.

Added MAVLink YAML elements.
rotorman added a commit to rotorman/edgetx that referenced this pull request Feb 6, 2022
The MAVLink specific settings are not yet implemented in YAML, otherwise tested on RM TX16S with DragonLink and ArduPilot.
Also for the added submodules, need to revert to using older versions from PR EdgeTX#150 and not their current heads.
rotorman added a commit to rotorman/edgetx that referenced this pull request Feb 6, 2022
The MAVLink specific settings are not yet implemented in YAML, otherwise tested on RM TX16S with DragonLink and ArduPilot.
Also for the added submodules, need to revert to using older versions from PR EdgeTX#150 and not their current heads.
rotorman added a commit to rotorman/edgetx that referenced this pull request Feb 6, 2022
* Merged EdgeTX PR EdgeTX#150
* Added MAVLink YAML elements.
* Set fastmavlink to be8b000 and mavlink to 37a6e90.
@pfeerick pfeerick modified the milestones: 2.7, 2.8 Mar 8, 2022
@pfeerick pfeerick modified the milestones: 2.8, 2.9 Aug 3, 2022
@gagarinlg gagarinlg modified the milestones: 2.9, 2.10 Jan 18, 2023
@raphaelcoeffic raphaelcoeffic modified the milestones: 2.10, 3.0 Oct 11, 2023
@brainbubblersbest
Copy link

This PR seems to became to an Classic "Fritz Künkel Incident" where the Main Focus somehow centered around Personal preferences instead of the Significant Purpose this Work was meant for at the beginning.

Open Ardupilot Firmware Meets MavLink Routing on Open maintained Radio Firmware . Too good to be True.

Olliw still have my Respect, even when this part of Software is finally abandoned. 🤠

@raphaelcoeffic
Copy link
Member Author

The issue at hand currently is the following:

  • quite some work required,
  • no maintainer who would be actually using this, and thus, have an incentive in actively maintaining this part.

I don't doubt this would have quite some users, but history has proven that maintaining things none of the active devs is using himself can prove challenging. For this particular feature, there are additional challenges due to the extra setup effort required to be able to test anything.

@timtuxworth
Copy link

With ExpressLRS working on a MAVLink option (their mavlink-rc branch) - maybe people (developers) will start using it?

@catkira
Copy link

catkira commented Dec 17, 2023

olliw42 is just an offended child, I think you should get over it (close this PR), screw his code and start from new. It's not that much code after all and the 2nd version will be better for sure!

@pfeerick
Copy link
Member

You are welcome to express your opinion (although it is somewhat inflammatory and personally directed), but I for one do not agree with that. While we may have had our differences, olliw42 has been responsible for a lot of things mavlink, and both his viewpoint and experience in this matter should be respected.

@catkira
Copy link

catkira commented Dec 17, 2023

You are welcome to express your opinion (although it is somewhat inflammatory and personally directed), but I for one do not agree with that. While we may have had our differences, olliw42 has been responsible for a lot of things mavlink, and both his viewpoint and experience in this matter should be respected.

I think the review comments in the original PR made by @raphaelcoeffic were all reasonable. There was no reason to be offended by them. If a developer cannot handle that, he is useless or even harmful for a large scale project.

@gagarinlg
Copy link
Member

My 2 cents:
there is no need to be insulting.

Everything else is not to be discussed in an open medium until a solution can be found. We are open to get this finally done, together with everyone involved. When there are issues between certain people, there are ways to manage communications in a way that it can work.
I made an offer on rcgroups, we will see what is going to happen.

@catkira
Copy link

catkira commented Dec 17, 2023

and to be honest, a developer who wants to stay anonymous but is peacocky and wants to have a special treatment in the copyright header that is not compatible with the rest of the project. Red flag, this alone would be a reason for me to reject this PR. This is not how software development at scale works. You may lose something in the short term but at least you have a mode in which the project can work long term. Imagine someone like olliw wanted to do a contribution to the linux kernel - just unthinkable.

@gagarinlg
Copy link
Member

As said, nothing to discuss in public until it is resolved.

@gagarinlg
Copy link
Member

When someone feels the need to discuss, contact me on discord. My handle is (currently, it depends on CET/CEST) "Malte (GMT+1)"

@catkira
Copy link

catkira commented Dec 17, 2023

I think it's a phenomenon of the current zeitgeist that everybody tries to be over sensitive :) You don't have to go full elon and say GFYS to somebody but a little direct language and sticking to principles does not hurt imho. Afaik the reviewers here are mostly senior software developers in real life and have a lot of experience, the project is very lucky to have such qualified people. There is no reason why any PR should bypass quality control.

@brainbubblersbest
Copy link

brainbubblersbest commented Dec 20, 2023

And Tadaaa!
Multiplex RC ist back with Mission Planner Compatible Radio and GCS.
MAV-Link is Dead. Long live M[AV]-Link

Unbenannt
Multiplex-RC-Flyer
Beast-TX.com

@raphaelcoeffic
Copy link
Member Author

The fun part is that it says OpenTx core 😋

@gagarinlg
Copy link
Member

We should ask them for the source code of their obviously modified OpenTX variant

@brainbubblersbest
Copy link

brainbubblersbest commented Dec 21, 2023

You're right! Interesting. I wrote a comment in the manufacturers Forum many years ago, that if Multiplex will walk the same way as Graupner and Robbe, when in the same time ignoring Open Source, the company will dissapear. Then with Dronin around, some day I discovered the receiver size is more then the entire flight stack and switched to Frsky and r-xsr.

A few days ago I had a conversation with an mpx sales guy. Should I gather a few informations about the other implementation? I'm curious if Bertrand is aware of this. 😂 Screenshot_20231221_030616_Samsung Internet

@bsongis
Copy link
Contributor

bsongis commented Jan 23, 2025

You're right! Interesting. I wrote a comment in the manufacturers Forum many years ago, that if Multiplex will walk the same way as Graupner and Robbe, when in the same time ignoring Open Source, the company will dissapear. Then with Dronin around, some day I discovered the receiver size is more then the entire flight stack and switched to Frsky and r-xsr.

A few days ago I had a conversation with an mpx sales guy. Should I gather a few informations about the other implementation? I'm curious if Bertrand is aware of this. 😂 Screenshot_20231221_030616_Samsung Internet

No I was not aware until tonight!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: rebase A git rebase on top of the latest destination branch version is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.