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

add first version of gnns #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

PonomarevDA
Copy link
Contributor

@PonomarevDA PonomarevDA commented Mar 15, 2023

GNSS

I want to propose a new interface for GNSS instead of the existing one in UDRAL. Here I've tried to fix as many of Andrew Tridgell's notes as possible, taking into account the red lines marked by Pavel Kirienko. I also tried to pay attention to the performance and the bandwidth issue.

1. Andrew proposal

Based on Andrew Tridgell's suggestion we have the following structure:

  • uint3 instance
  • uint3 status
  • uint32 time_week_ms
  • uint16 time_week
  • int36 latitude
  • int36 longitude
  • float32 altitude
  • float32 ground_speed
  • float32 ground_course
  • float32 yaw
  • uint16 hdop
  • uint16 vdop
  • uint8 num_sats
  • float32 velocity[3]
  • float16 speed_accuracy
  • float16 horizontal_accuracy
  • float16 vertical_accuracy
  • float16 yaw_accuracy

As I understand the suggested structure is mostly based on AP_GPS/GPS_State. By the way, this is what PX4 has.

It is 56 bytes long (9 CAN-frames in Cyphal).

It is pretty rigid interface, but it was pointed out that it is really important for ArduPilot to parse the all the necessary for EKF data at once.

In Cyphal we don't need to have an instance id and it is expected to use SI.

2. Existing Cyphal GNSS

The current GNSS implementation in the custom ArduPilot branch is just a minimal implementation to make it work.
It is based on something I found in an old DS-015 discussion. I did not consider it to be a finished GNSS interface, and I agree that it is not optimal at all.

Now it is divided into 5 messages:

  • position with reg.udral.physics.kinematics.geodetic.PointStateVarTs (67 bytes)
  • yaw with uavcan_si_sample_angle_Scalar (11 bytes)
  • num sats with uavcan_primitive_scalar_Integer16 (2 bytes)
  • status with uavcan_primitive_scalar_Integer16 (2 bytes)
  • pdop with uavcan_primitive_scalar_Integer16 (2 bytes)

All these messages together are 94 bytes long or 15 CAN-frames. And it is missing lots of absolutely critical information.

3. Proposal

Let's just convert what Andrew suggested with respect to SI and remove the instance ID.

I've extended the suggested status with mode and sub_mode that we have in DroneCAN and packed it into the Status message.

I've optimized physics.geodetic.Point.0.1 to have float32 for altitude instead of float64.

It seems to be that ground_speed and ground_course are redundant because we already have velocity, so I've removed them.

As a result, we get a 58 byte message, which is exactly the same number of CAN frames as in the original message.

Please, check Gnss.0.1 for details.

4. Alternatives

ArduPilot uses 4 variables for speed, horizontal, vertical and yaw accuracy. PX4 does the same. Should we consider using the triangular covariance matrix or these 4 variables suit us?

Can we consider combining uint32 time_week_ms and uint16 time_week into a single uint48 time_week_ms? Or should we put it into a separated message with 2 fields. Is TOW better than UTC or TAI?

5. Further work

I'm assuming that RTK, MovingBaseline and other features could be further extended as separate messages.

@PonomarevDA
Copy link
Contributor Author

I will create a similar PR for ESC, taking into account the notes from the forum discussion.


ds015.physics.geodetic.Point.0.1 point
uavcan.si.unit.velocity.Vector3.1.0 velocity
uavcan.si.unit.angle.Scalar.1.0 yaw
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heading accuracy?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A different approach could be to separately send a relative position when it's available instead of just the dumbed down heading with no metadata.

Screenshot from 2023-03-15 16-03-32

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heading accuracy is just a little lower, it is called yaw_accuracy.
I've added RelativePosition.0.1 dsdl. We can send it if it is available as a separate message. Is it what you mean?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sending it separately (when available) makes sense.

@dagar
Copy link

dagar commented Mar 15, 2023

Feel free to ignore this as scope creep, but Ublox GNSS receivers do provide a full covariance matrix I'm interested in using.

Screenshot from 2023-03-15 15-59-32

Additionally random little fields for jamming indication and spoofing detection can be useful.

@PonomarevDA
Copy link
Contributor Author

I've added fields for jamming and spoofing based on what PX4 has in the Status.0.1 that is part of the general Gnss.0.1 data type. Gnss message is 59 bytes now, but still 9 CAN-frames.

@PonomarevDA
Copy link
Contributor Author

Could we consider sending the covariance in a separate message? ublox sends the covariance as a separate message anyway, it doesn't matter where we combine all the data together (on the node side or on the autopilot side).
Please, check Covariance.0.1. It is 24 bytes long or 4 CAN-frames.

@dagar
Copy link

dagar commented Mar 16, 2023

Could we consider sending the covariance in a separate message? ublox sends the covariance as a separate message anyway, it doesn't matter where we combine all the data together (on the node side or on the autopilot side). Please, check Covariance.0.1. It is 24 bytes long or 4 CAN-frames.

Yes that makes sense, packing it together atomically is a bit of a lie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants