OpenCyphal / public_regulated_data_types

Regulated DSDL definitions for Cyphal (standard and third-party)
https://nunaweb.opencyphal.org
MIT License
72 stars 96 forks source link

GLOBAL REVIEW OF DSDL DEFINITIONS #34

Closed pavel-kirienko closed 5 years ago

pavel-kirienko commented 7 years ago

As a reminder, the version 1.0 of the specification has not yet been released. Before the release, we need to walk over all of the messages defined in the namespace uavcan.equipment. to weed out the remaining weirdness and inconsistencies.

olliw42 commented 7 years ago
antoinealb commented 7 years ago

general concept for adjusting the parameters in the node. I don't think it is really user friendly to use a SLCAN adapter, or other dedicated hardware just for handling the parameters. I rather would consider something which e.g allows one to set them from a GCS, with the flight controller as bridge. And so on.

In my team we use a lot this feature, and I think it is very useful to have it as a standard in the UAVCAN protocol. What we did is that the parameters are stored in a key/value store to which UAVCAN is interfaced. This allows us to have several other interfaces (like a GCS, UART, etc) all working on the same set of parameters.

uavcan.equipment.ahrs.MagneticFieldStrength3 (@jschall suggested to make the message timestamped, I agree;

Also agree, I am facing this issue right now while implementing a compass.

jschall commented 7 years ago

uavcan.equipment.ahrs Why are magnetometer and IMU measurements under uavcan.equipment.ahrs and not under a uavcan.equipment.magnetometer and uavcan.equipment.imu?

Inertial data There's two use cases for inertial data: estimation and control. Estimators want integrated delta-velocities and delta-angles that are corrected for coning, sculling and rotation, and they want a hard guarantee that they have all data up to the current time. For the estimator use case, there needs to be a way to detect missed delta-velocities and delta-angles, and if a subscriber does miss any data, there should an interface for the subscriber to attempt to retrieve the missed data from the publisher.

Controllers want filtered angular velocities and accelerations, and they only care that they have the latest value. I do wonder if perhaps we should look into using some sort of delta compression, so that we can send the full-rate (e.g., 1khz) stream in blocks at a lower rate, and subscribers can decide how to filter the data. In fact, this could allow subscribers to perform coning, sculling, and rotation corrections, so that estimators and controllers would not need separate messages.

I think Paul Riseborough would likely be able to help us find solutions for this.

Currently, the uavcan.equipment.ahrs.RawIMU message has no sensor_id field, so there seems to be support for only one sensor on a network. It also use floating point values throughout, which I will talk about below. Perhaps this message should be mothballed as well?

Timestamps: Optionally timestamp basically everything that comes out of a sensor or estimator. It seems like we should be able to work out a way to do this with far less than 56 bits - we're probably good with ~100us precision on most timestamps, and the time between generating and sending the message could be required to be shorter than a few seconds. This means we could use e.g. bits [8, 23] of the timestamp, which will wrap in 128*2^15 microseconds ~= 4.2 seconds. If (e.g.) a magnetometer measurement is that old, something is totally and utterly broken.

Fixed-point: Generally get rid of floating-point and use fixed point instead, as it is more efficient. Perhaps add support for Q-format https://en.wikipedia.org/wiki/Q_(number_format) fields in the dsdl? One good example of a use for this is angles: Q1.N can represent numbers in the range [-1, 1-2^(-n)] with uniform precision of 2^(-n) - i.e., ~0.1 mrad for Q1.15 and ~25 mrad for Q1.7. This makes more efficient use of bits. Further, most of the sensors that we use (mems, magnetometers, etc) output 16-bit values with some arbitrary unit. e.g. for a MagneticFieldStrength message, if the requirement is to represent earth's magnetic field (~65 uT) plus some reasonable margin for bias error etc, with uniform precision:

uint16 timestamp_bits_8_to_23

uint4 sensor_id Q9_3[3] magnetic_field_uT // represents -256..255.875 uT with .125 uT (1.25 mGa) precision - better than ak09916

This fits in a single CAN frame without the covariance matrix. A separate message could exist for magnetometers that don't timestamp.

Representing orientations If we desire to represent an orientation, there are numerous options https://en.wikipedia.org/wiki/Rotation_formalisms_in_three_dimensions available to us. For the purposes of a communications protocol, I think we want to avoid quaternions as the are overdetermined (and thus waste bits in order to represent invalid quaternions), and instead use 3-2-1 intrinsic euler angles with a fixed-point representation. The user can convert to quaternions if desired (and some uavcan implementations could even provide convenience functions for doing so)

e.g. on uavcan.equipment.ahrs.Solution:

float16[4] orientation_xyzw

becomes:

void1 Q1_20 roll Q1_20 pitch UQ1_20 yaw

Each of those angles has a uniform resolution of 3 microradians.

Parameters Would be nice to have "modified and not stored" and "requires reboot" flags for parameters, for display purposes. Also, any reason why floating point values in parameters are not represented by 64 bits?

On Tue, Oct 3, 2017 at 5:35 AM, Antoine Albertelli <notifications@github.com

wrote:

general concept for adjusting the parameters in the node. I don't think it is really user friendly to use a SLCAN adapter, or other dedicated hardware just for handling the parameters. I rather would consider something which e.g allows one to set them from a GCS, with the flight controller as bridge. And so on.

In my team we use a lot this feature, and I think it is very useful to have it as a standard in the UAVCAN protocol. What we did is that the parameters are stored in a key/value store to which UAVCAN is interfaced. This allows us to have several other interfaces (like a GCS, UART, etc) all working on the same set of parameters.

uavcan.equipment.ahrs.MagneticFieldStrength3 (@jschall https://github.com/jschall suggested to make the message timestamped, I agree;

Also agree, I am facing this issue right now while implementing a compass.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/UAVCAN/dsdl/issues/34#issuecomment-333827802, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9_0eussAT0ZsUKxuVgZDph0LId3LjKks5soioNgaJpZM4O1r1C .

pavel-kirienko commented 7 years ago

Why are magnetometer and IMU measurements under uavcan.equipment.ahrs and not under a uavcan.equipment.magnetometer and uavcan.equipment.imu?

Please do submit a fix! The new magnetic field message mentioned above should go into a separate namespace as you suggested. The old ones should be declared deprecated.

For the estimator use case, there needs to be a way to detect missed delta-velocities and delta-angles, and if a subscriber does miss any data, there should an interface for the subscriber to attempt to retrieve the missed data from the publisher.

This would be over-engineering. The guarantees against data losses provided by CAN are likely sufficient for the task.

Currently, the uavcan.equipment.ahrs.RawIMU message has no sensor_id field, so there seems to be support for only one sensor on a network.

Lack of sensor_id means that we can support at most one IMU per node, not per network. However, it still might make sense to add the field.

It seems like we should be able to work out a way to do this with far less than 56 bits

We had this discussion in the chat room already. Overflowing timestamps introduce undesirable corner cases, and given the future support for CAN FD, the saves doesn't seem to worth the extra complexity. I propose to keep the current method of timestamping for now.

Fixed-point:

I understand the efficiency argument, but the overall approach goes against the core design goals of UAVCAN, especially those concerning simplicity. Use of plain floats greatly simplifies handling of data on the side of application, and encourages designers to use SI rather than custom units. For example, we turned down a proposition of a status message for internal combustion engines a while back specifically because the developers were relying on fixed point too much.

My proposition is to avoid any numerical representations other than integers and IEEE754, at least until a much stronger case in their favor is built.

For the purposes of a communications protocol, I think we want to avoid quaternions as the are overdetermined (and thus waste bits in order to represent invalid quaternions), and instead use 3-2-1 intrinsic euler angles with a fixed-point representation.

How do we handle the gimbal lock problem?

Would be nice to have "modified and not stored" and "requires reboot" flags for parameters, for display purposes. Also, any reason why floating point values in parameters are not represented by 64 bits?

The two flags you mentioned make sense. We can do that after https://github.com/UAVCAN/dsdl/issues/36 is implemented.

As for the lack of 64-bit floats: there's no strong reason why they are not supported. The reasoning was that parameters are mostly intended to be changed by humans, and a human would likely not benefit much from the high precision/resolution of 64-bit floats. Please describe your use case where you think 64-bit float parameters would be needed?

jschall commented 7 years ago

Please do submit a fix! The new magnetic field message mentioned above should go into a separate namespace as you suggested. The old ones should be declared deprecated.

I'd like to really, fully understand how the _id fields actually work first, and I'm still a bit hesitant about the 56-bit timestamp thing.

This would be over-engineering. The guarantees against data losses provided by CAN are likely sufficient for the task.

I'm not so sure about this. CAN/UAVCAN is still vulnerable to dropping messages over things like full buffers.

We had this discussion in the chat room already. Overflowing timestamps introduce undesirable corner cases, and given the future support for CAN FD, the saves doesn't seem to worth the extra complexity. I propose to keep the current method of timestamping for now.

I think efficiency will be desirable even with CAN FD. CAN FD adoption among UAVCAN users is also probably a few years away.

I understand the efficiency argument, but the overall approach goes against the core design goals of UAVCAN, especially those concerning simplicity. Use of plain floats greatly simplifies handling of data on the side of application, and encourages designers to use SI rather than custom units. For example, we turned down a proposition of a status message for internal combustion engines a while back specifically because the developers were relying on fixed point too much.

My proposition is to avoid any numerical representations other than integers and IEEE754, at least until a much stronger case in their favor is built.

We can have more precision while packing more data into messages. What's not to like? I would tend to disagree that use of plain floats greatly simplifies handling of data - half precision floats are not commonly supported natively, and single precision float is not supported natively on all platforms. Integers are supported natively on basically all platforms, and floats are not. I've done projects on microcontrollers where it was necessary to use fixed-point math in order to save flash. As far as encouraging SI units goes, if the means to specify the fixed-point format are provided, then developers can select a format that allows them to use SI units.

There are a few cases where the properties of floating point are desirable

I feel like there's some level of dissonance between using things like bit fields and tail array optimization, that take effort to implement and use, and then using floating point all willy-nilly.

How do we handle the gimbal lock problem?

Euler angles have no problem representing any orientation. Gimbal lock becomes an issue only when you try to use euler angles in differential equations. So, just convert the euler angles to quaternions before using them in algorithms. One disadvantage of this is that trig functions are required, but that won't be a problem on most platforms, and on platforms where it is a problem, it is at least a very solvable one.

As for the lack of 64-bit floats: there's no strong reason why they are not

supported. The reasoning was that parameters are mostly intended to be changed by humans, and a human would likely not benefit much from the high precision/resolution of 64-bit floats. Please describe your use case where you think 64-bit float parameters would be needed?

I can't think of a specific/strong need for 64-bit floats, but I also don't see a strong reason not to support them instead of 32-bit floats - they can represent everything that a 32-bit float can represent and more.

On Thu, Oct 5, 2017 at 7:52 AM, Pavel Kirienko notifications@github.com wrote:

Why are magnetometer and IMU measurements under uavcan.equipment.ahrs and not under a uavcan.equipment.magnetometer and uavcan.equipment.imu?

Please do submit a fix! The new magnetic field message mentioned above should go into a separate namespace as you suggested. The old ones should be declared deprecated.

For the estimator use case, there needs to be a way to detect missed delta-velocities and delta-angles, and if a subscriber does miss any data, there should an interface for the subscriber to attempt to retrieve the missed data from the publisher.

This would be over-engineering. The guarantees against data losses provided by CAN are likely sufficient for the task.

Currently, the uavcan.equipment.ahrs.RawIMU message has no sensor_id field, so there seems to be support for only one sensor on a network.

Lack of sensor_id means that we can support at most one IMU per node, not per network. However, it still might make sense to add the field.

It seems like we should be able to work out a way to do this with far less than 56 bits

We had this discussion in the chat room already. Overflowing timestamps introduce undesirable corner cases, and given the future support for CAN FD, the saves doesn't seem to worth the extra complexity. I propose to keep the current method of timestamping for now.

Fixed-point:

I understand the efficiency argument, but the overall approach goes against the core design goals of UAVCAN, especially those concerning simplicity. Use of plain floats greatly simplifies handling of data on the side of application, and encourages designers to use SI rather than custom units. For example, we turned down a proposition of a status message for internal combustion engines a while back specifically because the developers were relying on fixed point too much.

My proposition is to avoid any numerical representations other than integers and IEEE754, at least until a much stronger case in their favor is built.

For the purposes of a communications protocol, I think we want to avoid quaternions as the are overdetermined (and thus waste bits in order to represent invalid quaternions), and instead use 3-2-1 intrinsic euler angles with a fixed-point representation.

How do we handle the gimbal lock problem?

Would be nice to have "modified and not stored" and "requires reboot" flags for parameters, for display purposes. Also, any reason why floating point values in parameters are not represented by 64 bits?

The two flags you mentioned make sense. We can do that after #36 https://github.com/UAVCAN/dsdl/issues/36 is implemented.

As for the lack of 64-bit floats: there's no strong reason why they are not supported. The reasoning was that parameters are mostly intended to be changed by humans, and a human would likely not benefit much from the high precision/resolution of 64-bit floats. Please describe your use case where you think 64-bit float parameters would be needed?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/UAVCAN/dsdl/issues/34#issuecomment-334489714, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9_0USDfgPEVZOd-FBSUaDyBJLaI1ZLks5spO0mgaJpZM4O1r1C .

lthall commented 7 years ago

Hi all, I understand that this is where I should be putting feedback on the CAN stuff. My only comment so far is about the ESC feedback.

The main feature I would like to ensure is present in the ESC communication is feedback of the maximum available throttle. It is important that this be in the similar precision as the commanded throttle but I would suggest we make it the same.

There are lots of reasons an ESC may limit the maximum throttle output. Some obvious ones are to protect against over current during recovery from low RPM or high loads, or to limit temperature build up.

If the autopilot can collect the maximum throttle available from each ESC it can account for this in the output mixer. If it can't collect this data it must wait until the limit causes a disturbance before it can correct for it via the control loops.

The other issue is the maximum throttle feedback may be needed at a much higher rate than voltage and current feedback. If the overhead of the additional data in the message is too much then we may need to have two messages for esc's.

Anyway, I hope this can be accommodated.

EShamaev commented 6 years ago

@lthall Can you please provide example for what autopilot should do if it knows that the maximum throttle is limited?

lthall commented 6 years ago

Hi EShamaev,

EShamaev commented 6 years ago

1st and 2nd is done by autopilot's control loops without the need for feedback from ESC. 3rd is a good idea, however can be easily calculated from difference in demand and actual value and if such message is given it should be acknowldeged on GCS and taken in consideration.

But I agree that adding more data to Status of ESC is needed and maximum available throttle is not the only thing that needs to be added.

lthall commented 6 years ago

EShamaev, I would not have put points 1 and 2 if I thought they could be done effectively without feedback from the ESC. The first point can be addressed by the control loops after the aircraft has rolled, pitched, or yawed. So an ESC limiting the output thrust will result in anything from a dip in an arm or a full flip of the aircraft before the control loops can take care of it. 2 can not be done by the control loops at all and would require quite a complex calculation that requires knowledge of the available thrust of each motor.

I get the feeling you think I am stupid and provide answers without thinking.

EShamaev commented 6 years ago

@lthall Please do not get that feeling. It's not true.

But I think that in order that control loops could behave that way they would need more parameters to be setup and if they are setup not correctly might lead to much more serious problems. Also the control loops (if designed correctly) are able to cope with loss of thrust from one of the motors if there is a possible way to do so. 1st and 2nd are already implemented in Ardupilot's control loops for example if I remember correctly.

However I fully agree that Status of ESC should be extended.

lthall commented 6 years ago

@EShamaev As the primary author of the motor mixers and attitude controllers in ArduCopter since release 2.9 (something like 5 years now), I understand better than most what a well designed attitude controller can and can not do without ESC feedback and what it could do if it did have ESC feedback. I have been thinking about the implications of CAN based ESC's for well over two years and I have plans for what I can do to improve the performance of the attitude controllers, motor mixers and multi-rotors in general if given the required information.

None of the features above require any parameters to be set by the user or pose additional risk.

The current control loops can maintain attitude control without feedback from the ESC's if there is enough thrust from the lost motor or enough redundancy in the multi-rotor configuration. In the case of a hex or octo this is at the expense of reduced maximum thrust. Working from memory, the maximum thrust without ESC feedback in the event of total loss of a motor is something like 50% compared to 30% if there is feedback (point 2). I will get the exact figures when I get home tonight. However, this correction can only be done by the PID loops after the aircraft's attitude is compromised by the thrust loss. The magnitude of the thrust loss and the time over which that loss happens combined with the state of the aircraft at the time may mean the difference between a complete loss of the aircraft or personal injury or simply a small sag to one side. With fast feedback from the ESC I am able to correct for the issue at the motor mixer stage before any attitude disturbance is registered by the PID loops.

As I have said already, none of the features I mentioned above are possible without ESC feedback and therefore have not been implemented in Arducopter or any other flight controller without esc feedback. (A flight controller could take a reliable guess at 3 if all it ever did was hover in calm conditions).

Do I understand you correctly when you say "However I fully agree that Status of ESC should be extended." that you mean you can see why a separate small, efficient, high speed data packet is needed to provide near instantaneous output feedback from the ESC? If so, thank you for taking the time to talk this through with me!!

EShamaev commented 6 years ago

Yes, the feedback for thrust limit is not the only thing that I see should be added to ESC status.

kd0aij commented 6 years ago

I agree with @jschall that orientations should be specified using fixed point Euler angles.

EShamaev commented 6 years ago

The only advantage to have Euler angles is that they are easier to read by human.

However for camera's gimbal control quaternions give much more than can be seen from first. When camera gets a new position to turn to, if all calculations are done in quaternions you will get a much more smooth and expected interpolation between positions. Then the conversion from quaternion to Euler angle is more simple operation than from Euler angle to quaternion. Also the quaternion heading for gimbal is natural because in most cases as the camera's line of sight needs to be stabilized respective to earth, all calculations are still going to be in quaternion to overcome gimbal lock.

kd0aij commented 6 years ago

As @jschall pointed out, floating point quaternions waste bits... and that is contrary to the objective of low latency and high throughput. And what is the orientation precision of a float16 quaternion? I don't see it mentioned anywhere in the spec, and float16 is not a numerical format that I've ever seen used for quaternions.

EShamaev commented 6 years ago

I would agree that a fixed point quaternion would do better bitwise, but there is a balance between need to have custom types and having standard notation.

The precision is around 0.36 degree if float16 is used for quaternion. It is nearly the same if the same data type is used to represent Euler angles.

However I agree that using 16 bit custom type we could have around 0.005 degree of precision with Euler angles. The question is if it is really needed and if the overhead of converting from custom type to 32bit double quaternion is worth it for camera gimbal.

What can be done is a fixed point 16 bit type (0=0, 1=32767) to represent quaternion's values which will be converted to double after reception.

mjs513 commented 6 years ago

I am still getting into UAVcan but a couple of messages that I have been playing with are GNSS, IMU and now gimbal since I just picked up a SimpleBGC 2-axis gimbal. Just from some initial observations I would say that for the gimbal I would agree with olliw42. Over the next couple of days I will be experimenting with this and can probably feed you all more info. But what I see of the Mavlink message that looks about right - https://mavlink.io/en/protocol/gimbal.html. One thing that is missing is the ability just to send angles to the controller to control the gimbal.

As for the IMU data I have the Node due the raw imu data analysis and forward that data for action specifically is Roll, pitch, yaw and heading. Depending on the use case it would be in degrees and/or quaternions if I want to do something like Kalman filtering with GPS/IMU data combined.

As for the GNSS I see no problem so far with that message structure.

Respectfully Mike

kjetilkjeka commented 6 years ago

We're currently working on a system that will formalize stabilization, compatibility, deprecation and early experimentation with DSDL definitions together with other important changes to the protocol. We're a bit "all hands on deck" with this at the moment, so I believe non-critical changes to concrete types will probably be delayed. The comments you leave here will, of course, be highly appreciated when the right time comes.

Even though a lot of the changes to the core protocol has been accepted there are a few things related to how DSDL is namespaced, changed and deprecated remaining. If you can (if you want to) follow/participate in the protocol update effort by following here https://github.com/UAVCAN/specification/issues/2.

pavel-kirienko commented 5 years ago

Done in the branch uavcan-v1.0, please review and open issues/PRs as necessary: https://github.com/UAVCAN/dsdl/tree/uavcan-v1.0

Context: https://forum.uavcan.org/t/stockholm-summit-recap/170