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

Mavlink additions #1422

Merged
merged 12 commits into from
Nov 17, 2015
Merged

Mavlink additions #1422

merged 12 commits into from
Nov 17, 2015

Conversation

flixr
Copy link
Member

@flixr flixr commented Nov 14, 2015

Add extra functionality of #1407 to current mavlink module.

  • fix sending of some mavliink messages (MAVLinkSendMessage() was missing)
  • send GPS_STATUS and VFR_HUD
    • and actually fill in more fields
  • parse MISSION_ITEM message to update waypoints
  • add the Paparazzi specific SCRIPT_ITEM messages

Looks to me like this now added the same functionality as #1407 with two minor differences:

  • no time-outs on the mission or script protocol, don't see how they would be relevant anyway since you can only change existing waypoints/blocks
  • MISSION_ITEM message is sending waypoint in MAV_FRAME_LOCAL_ENU instead of MAV_FRAME_GLOBAL only for fixedwing
    • this has the advantage of having higher precision (representing lat/lon in float is not nice)
    • still works correctly with the android app?

This should keep all the already existing functionality that #1407 doesn't have:

  • works for UART, UDP, USB serial
  • more messages supported
  • support of changing settings
  • override RC channels
  • support rotorcraft and fixedwing (except for waypoint changes)
  • more accurately report current mode, type, etc...

One thing I was wondering about: why add a len field to the SCRIPT_ITEM message? It seems to be totally superfluous to me...

@dewagter @LodewijkSikkel what do you guys think? And plz test with your setup...

makes it possible to change waypoints when using the rotorcraft firmware
- makes it possible to get and change the flight plan blocks

alternative implementation to #1407
@flixr flixr added the Module label Nov 14, 2015
@flixr
Copy link
Member Author

flixr commented Nov 15, 2015

Btw, to use the mavlink module with UDP, this should do the trick:

<define name="UDP1_PORT_OUT" value="14550"/>
<define name="UDP1_PORT_IN" value="14555"/>
<configure name="MAVLINK_PORT" value="UDP1"/>

flixr and others added 4 commits November 15, 2015 13:00
sending lat/lon as float is actually a bad idea,
but it seems that most GCSs don't understand the MISSION_ITEM_INT or LOCAL_FRAME_ENU
- don't use timer callbacks as they are called from system context
- remove duplicate waypoints from mission manager
- waypoint protocol:
  - start waypoint write/update transaction upon receiving MISSION_COUNT message
  - request waypoints

Seems to work fine with e.g. QGroundControl now
@flixr flixr added this to the v5.8 milestone Nov 15, 2015
@flixr
Copy link
Member Author

flixr commented Nov 15, 2015

Updated to integrate the missionlib of @LodewijkSikkel and fixed/improved:

  • don't use timer callbacks as they are called from system context
  • remove duplicate waypoints from mission manager
  • waypoint protocol:
    • start waypoint write/update transaction upon receiving MISSION_COUNT message
    • request waypoints
    • send ack only after all waypoints are transferred

Updating waypoints seems to work fine with e.g. QGroundControl now.

Question: do you want/need to allow updates of single waypoints via the app?
From the protocol it looks like this is not how it's supposed to be used....

@dewagter
Copy link
Member

You also need a working version of this define:

  <define name="UDP1_HOST" value="192.168.42.2"  type="string[]"/>

(This particular one does not seem to compile). command-line>:0:11: error: too many decimal points in number

@flixr
Copy link
Member Author

flixr commented Nov 16, 2015

I think the type is currently only parsed for sections, not the firmware... also we should probably not require the UDPx_HOST to be a string (and use STRINGIFY in the code).

But for now you can specify the host as we have done before:

<define name="UDP1_HOST" value="\\\"192.168.42.2\\\""/>

@dewagter
Copy link
Member

@flixr This update looks very nice. Thank you for all this work.

Unfortunately it does not work with the paparazzi app (the only mavlink app that can call paparazzi-flightplan-blocks). Debugging this is very hard as this is the code of 3 other people. Any idea what order of messages could be different than with the other branch?

@dewagter
Copy link
Member

Yes, It finally works!

  • remove video_streaming_rtp or change port to non 5000

  • change airframe to ID nr 1 !!!!!

  • add to your airframe modules

    <load name="mavlink.xml">
    <define name="UDP1_PORT_OUT" value="5000"/>
    <define name="UDP1_PORT_IN" value="5000"/>
    <define name="UDP1_HOST" value="\"192.168.42.2\""/> or the IP if your mobile device when running a simulation
    <configure name="MAVLINK_PORT" value="UDP1"/>
    </load>

  • connect your mobile device to the bebop before anyone else (to get ip 192.168.42.2) / check your sim and tablet are on the same local network but not with the same IP

  • Install and run: https://play.google.com/store/apps/details?id=com.pprzservices

  • Install and run: https://play.google.com/store/apps/details?id=com.gcs

@flixr
Copy link
Member Author

flixr commented Nov 16, 2015

The paparazzi AC_ID is sent as system_id in mavlink... the app should not assume this is 1.

Any comments on the questions raised above?

@LodewijkSikkel
Copy link
Contributor

This is a bug that should have been fixed in de GCS app. The student that is working on it should look into it.

On 16 Nov 2015, at 13:50, Felix Ruess <notifications@github.commailto:notifications@github.com> wrote:

The paparazzi AC_ID is sent as system_id in mavlink... the app should not assume this is 1.

Any comments on the questions raised above?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1422#issuecomment-157018049.

@flixr
Copy link
Member Author

flixr commented Nov 16, 2015

Open questions:

  • why have a len field in the SCRIPT_ITEM message?
  • really (mis?)use SCRIPT_ITEM to indicate current block?
  • use of different position representation in app
    • use MISSION_ITEM_INT messages in app (lat/lon in float has bad precision)
    • or use MISSION_ITEM with local frame
  • allow to update a single waypoint? Currently you have to send the whole list as the protocol specifies
  • allow writing of waypoints for fixedwing? Or wait until fixedwing supports our new waypoint API?

@dewagter
Copy link
Member

  • len was the string length of the name. can be removed and replaced by zero character
  • adding a SCRIPT_CURRENT sound like a clean approach if waypoints also have this
  • BUT please note that changing the paparazzi side only is breaking the whole point of this pull request:
  • allow update of a single waypoint is very important for paparazzi. I know mavlink does not have this, but we DO need it.
  • other 2 points I did not investigate.
    • IMHO, in general I would say that ground stations best communicate in LLH, but not in float-llh: either int-llh or double-llh.
    • Again IMHO: I think fixedwing is close to being integreted in rotorcraft (since rotor-intermcu) so I'd rather add the missing control and flightplan in rotorcraft than rewrite fixedwing WP.
  • side note: I think this module has all the functionality now of the APP_SERVER used in pprz_ondroid: listing blocks and activating them, listing all settings and WP besides position and attitude.

@flixr
Copy link
Member Author

flixr commented Nov 16, 2015

Of course changing the added paparazzi specific mavlink messages means updating it everywhere... that is exactly why I would rather like to do it now than later...

allow update of a single waypoint is very important for paparazzi. I know mavlink does not have this, but we DO need it.

I guess it would work that if you receive a MISSION_ITEM without a previous MISSION_COUNT you could update that waypoint directly and ack it.
Will do that... hopefully it doesn't break other stuff...

if a MISSION_ITEM is sent from the GCS without a MISSION_COUNT first,
then simply directly update that waypoint and answer with MISSION_ACK
@flixr
Copy link
Member Author

flixr commented Nov 17, 2015

So we merge this as is and make another PR for updating the SCRIPT messages?

@dewagter
Copy link
Member

Yes, I will try to find manpower to update the app a.s.a.p., but please
already merge this working version.

We can start a new pull request with your protocol update suggestions
immediately and merge that as soon as the corresponding apps support it.
On Nov 17, 2015 11:51 AM, "Felix Ruess" notifications@github.com wrote:

So we merge this as is and make another PR for updating the SCRIPT
messages?


Reply to this email directly or view it on GitHub
#1422 (comment).

flixr added a commit that referenced this pull request Nov 17, 2015
Mavlink additions

- fix sending of some mavlink messages (MAVLinkSendMessage() was missing)
- send GPS_STATUS and VFR_HUD and SYSTEM_TIME
- waypoint protocol:
  - parse MISSION_ITEM_INT message to update single waypoints
  - only for rotorcraft (since it used the waypoint interface, see #981)
  - start waypoint write/update transaction upon receiving MISSION_COUNT message (with requesting of waypoints)
  - send ack only after all waypoints are transferred
- add the Paparazzi specific SCRIPT_ITEM messages

Updating waypoints seems to work fine with e.g. QGroundControl now.
@flixr flixr merged commit b69aa2d into master Nov 17, 2015
@flixr flixr deleted the mavlink_additions branch November 17, 2015 22:24
@dewagter dewagter mentioned this pull request Nov 21, 2015
3 tasks
@flixr flixr mentioned this pull request Nov 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants