edgetx icon indicating copy to clipboard operation
edgetx copied to clipboard

Mavlink support

Open raphaelcoeffic opened this issue 3 years ago • 39 comments

This PR continues the MAVLink integration of #122.

TODOs

Cosmetics / Code structure

  • [x] Remove Olli's markers (// OW / // OWEND).
  • [x] Use the standard GPLv2 boiler plate instead of Olli's copyright notice.
  • [x] Remove useless USB_MAVLINK_MODE and USB serial description specific to MAVlink (USB serial is just "USB serial").
  • [x] Remove MAVLink includes from radio/src/opentx.h and add to appropriate files that need it.
  • [x] Move unrelated function in radio/src/lua/api_mavlink.cpp to somewhere else (does anyone really need this?).
  • [x] 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.
  • [x] Check why g_eeGeneral.mavlinkExternal == 1 should disable external module functionality (probably not needed).
  • [x] 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).
  • [x] Check why uint16_t telemetryStreaming (normally uint8_t).

raphaelcoeffic avatar Jun 05 '21 08:06 raphaelcoeffic

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

:-1:

olliw42 avatar Jun 05 '21 08:06 olliw42

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 avatar Jun 05 '21 08:06 raphaelcoeffic

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.

raphaelcoeffic avatar Jun 05 '21 09:06 raphaelcoeffic

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 avatar Jun 05 '21 09:06 rotorman

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:~~ https://github.com/EdgeTX/edgetx/pull/36#issuecomment-842437807

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

rotorman avatar Jun 05 '21 09:06 rotorman

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

olliw42 avatar Jun 07 '21 05:06 olliw42

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.

raphaelcoeffic avatar Jun 07 '21 05:06 raphaelcoeffic

Tested 7a19a3ae 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 avatar Jun 08 '21 07:06 rotorman

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).

rotorman avatar Jun 08 '21 08:06 rotorman

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 avatar Jun 08 '21 08:06 raphaelcoeffic

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.

raphaelcoeffic avatar Jun 08 '21 08:06 raphaelcoeffic

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

olliw42 avatar Jun 08 '21 08:06 olliw42

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.

raphaelcoeffic avatar Jun 08 '21 09:06 raphaelcoeffic

@olliw42 By looking at diff https://github.com/EdgeTX/edgetx/pull/150/commits/f7c0036d89725a6d6422066732360402b6e56246 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?

rotorman avatar Jun 08 '21 09:06 rotorman

@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 avatar Jun 08 '21 09:06 raphaelcoeffic

@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.

raphaelcoeffic avatar Jun 08 '21 09:06 raphaelcoeffic

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.

pfeerick avatar Jun 08 '21 09:06 pfeerick

Tested 04a91fb4 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)

rotorman avatar Jun 08 '21 11:06 rotorman

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

olliw42 avatar Jun 08 '21 11:06 olliw42

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

rotorman avatar Jun 08 '21 11:06 rotorman

@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)

olliw42 avatar Jun 08 '21 11:06 olliw42

@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!

rotorman avatar Jun 08 '21 12:06 rotorman

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.

olliw42 avatar Jun 08 '21 12:06 olliw42

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?

rotorman avatar Jun 08 '21 12:06 rotorman

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

olliw42 avatar Jun 08 '21 12:06 olliw42

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.

rotorman avatar Jun 08 '21 12:06 rotorman

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.

brainbubblersbest avatar Jun 09 '21 19:06 brainbubblersbest

@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.

raphaelcoeffic avatar Jun 09 '21 20:06 raphaelcoeffic

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.

brainbubblersbest avatar Jun 09 '21 22:06 brainbubblersbest

from which you can learn that EdgeTx is not leading edge as it claims to be

they don't deliver to the users fully functioning features just because of developer affectedness like that a debug functionality is not to their liking or serial port drivers are not as nice as they wished them to be

I fully agree that some solutions of mine are not the best code wise, and I would have loved if I would have been able to do better, but users give a dam about this, they want the features, working features, and the solutions available today do already one thing: they work and deliver.

In remarkable contrast, in many other cases, code solutions which are just "not so nice" from any software engineering point of view are happily accepted to the code. Not nice, but they do the job, for now, so accepted. Not so here.

Concerning the external bay feature, you know that I was not so enthusiastic when you first suggested it, but I reverted my opinion, and publicly confessed so. It is a most important feature ... and from the very first second on I loved it and would not want to miss anymore ... this is simply the future ... and will open all sorts of super convenient solutions for users, many possible DIY. But not with EdgeTx. If you want leading edge you need to resort to my branch.

It's really sad. The MAVLink addition (and what it all came with) is likely by far the largest single novel addition, and the only novel addition of that magnitude so far, and likely also by release time ... and already in this very first biggest example where a leading edge attitude could have been demonstrated the EdgeTx project miserably fails ... because of developer affectedness and stubborness.

you are correct, the hardware is DIY no issue another excellent hardware base you've not mentioned is the siyi fm30

olliw42 avatar Jun 10 '21 04:06 olliw42