common: allow negative temperatures for ESC_INFO message

[cmic0]

This PR changes the temperature range of ESC_INFO to allow negative temperatures.

[cmic0]

@julianoes @MaEtUgR FYI

[hamishwillee]

So this changes ESC temperature information in WIP message from range 0 - 255 Celsius (uint8_t degC) to +/- 32767 Celsius (int16_t degC), with units only whole numbers.

• What is the range you're actually after?
• Is whole number sufficient resolution? You might consider using cdegC unit - which would give you range +/- 327 to 2 decimal places.
• This is a breaking change to the message. What is the implementation status of this message? I.e. Is this in production devices? What will be broken? Are you happy to fix up current usage in PX4/QGC?
• When can we move this from WIP to final?

[auturgy]

@hamishwillee 's comments/queries should be addressed.
ArduPilot doesn't use this message - as discussed prior to this message being created, we already had https://github.com/ArduPilot/mavlink/blob/ac05b73aca5039815543c07cefb44b81865979af/message_definitions/v1.0/ardupilotmega.xml#L1635 and haven't changed.

[auturgy]

A bitfield as an extension might achieve the same intent, defaulting to 0 as a positive temp - ie [0000] - which should then be trimmed. I believe that this would make this a non breaking change. Whilst it is WIP, and hence breaking changes permitted, my understanding is that this is implemented in the wild, ie PX4 stable releases (https://github.com/PX4/PX4-Autopilot/blob/9e4a04f58ab655c5129c0349c69cf70f472a3c51/src/modules/mavlink/mavlink_messages.cpp#L74), so regardless of our intent here in MAVLink, it can't be trivially broken.

[hamishwillee]

@cmic0 I re-read my notes from the devcall. They indicate that there is only very weak justification for negative temperatures, and that despite the fact that this is WIP, that it is already in a released version of PX4 and ROS. The proposal is therefore a breaking change.

The dev call essentially said:

• If you're going to do this, a mavlink 2 extension of a bitfield is one way you can do it compatibly - ie map bits with value 11 to indicate values in the temperature array represent negative value. You might alternatively have a separate array for negative temperatures, though that would be less efficient, particularly if we extend this again later.
• Contact the original author and ROS to let them know/discuss impacts.

Personally, on further consideration, I am somewhat opposed to this change. If we add a bit field we significantly increase the complexity of the implementation for users for a use case for which we have no compelling justification. I don't think it is worth it.
On the other hand, if you are willing to manage a breaking change to the temperature field - which will require the agreement/management of the affected parties - then it is worth it to have a single field that is simple to implement and understand.

Hope that makes sense. Either way the first step is to talk to ROS and the original author. It might be this could be made as a breaking change and we suck up the hassle - if there are only a few users "in practice". After all, that is why WIP exists.

FYI @auturgy @julianoes

[cmic0]

I think in general having a field definition for temperature which not supports negative temperatures is wrong.
Probably this was never a problem as ESCs capable of providing telemetry started to be widely adopted in the industry just recently.

I had a look at the lower lever protocols adopted for ESC communication like UAVCAN (v0) and DSHOT and they both support negative temperatures, so with the current message definition (and the PX4 implementation) we're just truncating relevant information.

Looking further ahead, knowing the exact temperature of the ESCs/Motors can be really important in applications where fuel/gas engines are being used on the vehicle.

On the other hand, if you are willing to manage a breaking change to the temperature field - which will require the agreement/management of the affected parties - then it is worth it to have a single field that is simple to implement and understand.

I do agree on this path. In terms of breaking change i will keep care of updating the PX4 side as well.

@RicardoM17 seems that you were one of the adopters of this message for your MAVROS offboard control, were you maybe aware of other projects adopting it?

[RicardoM17]

Hi @cmic0 .

In the original PR (#1322 ) adding negative temperatures was discussed. I believe that the main conclusion was that for a first implementation using only positive temperatures was good enough, but it was also something that could be revisited in the future (like now). I was also leaning towards including this but the discussion was already too long so it was merged as is.

From our side this won't break the workflow we implemented (though I'm no longer in that project). I was also not aware of any other projects using it, so it shouldn't be a problem for that. Up to this point I approve all of these changes.

That being said I can add one suggestion. Since you are changing from uint8 to int16 to allow for the negative range we might want to consider changing it in a way that it also increases resolution which was something that was also discussed at some point. With an int16 representing degC we will be using around 0.5% of the range at best which seems like a huge waste to me.

The ardupilot had a similar implementation using 1/10 or 1/100 of a unit instead (I believe for voltage). This can easily fit in a int16 for example.

Also if this does move forward let me know so that I can make the changes to update this on the MAVROS side.

[cmic0]

Based on the messages above I've just updated this PR to have the temperature field defined as int16, furthermore i also added as extension the motor_temperature field as this is something we should also consider for future use cases.

In both the cases i am using the cDegC unit to achieve +/- 327 to 2 decimal places as @hamishwillee suggested.

Thoughts?
@RicardoM17 @hamishwillee @julianoes @auturgy

[hamishwillee]

1. You have "Temperature measured by each ESC" and "Motor temperature measured by each ESC.". For the first one, do you mean "Temperature OF each ESC"? [ie the first is the ESC, the second is the motor itself?]
2. If you're breaking the message anyway, then you don't need to add the new motor temperature as an extension - so you'd remove that.
3. I'm OK as you guys have all agreed to "manage" the change, and as far as we know are the only users. BUT we need an implementation in a timely manner/prototype - can you guys do that before we merge?
4. I am assuming that the maximum heat of a motor can never reasonably exceed 327C - right?
5. When this goes in, I would like the change to be final - i.e. removing WIP. The WIP is not supposed to last forever - it is there for a prototyping period, which IMO this should be the end of. OK?

@auturgy You OK with the design of the new message? I know you don't use it in ArduPilot, but "would you be OK with it if you did"?

[auturgy]

I'd like to see an argument for including the motor temperature in this message rather than separate it out: mainly because it will rarely be used and adds an array that can't be trimmed.

[cmic0]

@hamishwillee

1. You have "Temperature measured by each ESC" and "Motor temperature measured by each ESC.". For the first one, do you mean "Temperature OF each ESC"? [ie the first is the ESC, the second is the motor itself?]

Yes - will be corrected.

2. If you're breaking the message anyway, then you don't need to add the new motor temperature as an extension - so you'd remove that.

Good point - will fix.

3. I'm OK as you guys have all agreed to "manage" the change, and as far as we know are the only users. BUT we need an implementation in a timely manner/prototype - can you guys do that before we merge?

Fine for me - once we're settled on the specification i'll open a PR on PX4 side before merging here.

4. I am assuming that the maximum heat of a motor can never reasonably exceed 327C - right?
   You are assuming right. Actually i've just realized that for fuel engines there is the EFI_STATUS which is really covering well fuel engines applications.
   This simplifies things here as the motor_temperature field in this PR can take care of the electric motors where the maximum temperature is usually defined that the working temperatures of neodymium magnets (for example, the most used ones, N types, have a max temp of 80deg).

To maybe address @auturgy observation, I could define the temperature as for other MAVLink types where the field is populated with 0 if motor_temperature is not provided, and fill with 0.01C if the motor is actually at 0C. This should enable message trimming, thoughts?

PS: also added ESC_FAILURE_MOTOR_OVER_TEMPERATURE to ESC_FAILURE_FLAGS

[hamishwillee]

To maybe address @auturgy observation, I could define the temperature as for other MAVLink types where the field is populated with 0 if motor_temperature is not provided, and fill with 0.01C if the motor is actually at 0C. This should enable message trimming, thoughts?

That would actually not allow trimming unless the motor_temperature was indeed an extension field - because the ordering of the fields pushes smallest ones last, and these are what get trimmed. And we don't want it to be an extension field because extension fields are the only ones checked for compatibility. Also because as soon as you further extend the trimming doesn't happen.

An (undesirable) option would be a configuration field that states the temperature is motor or ESC. So on occasion you could send this with motor temp <field type="uint8_t" name="temp_type" instance="true">If 0, the temperature is ESC temp, if 1, it is motor temp.</field>

Or another message for the motor temperature, when you discover that you actually need it. As suggested by @auturgy

@auturgy This is the low rate message, which is already quite large. I don't argue that we generally don't want bigger messages, but is this really such an issue?

PS: also added ESC_FAILURE_MOTOR_OVER_TEMPERATURE to ESC_FAILURE_FLAGS

Excellent idea whatever else we do here.

[auturgy]

I’d prefer a separate message, and I’d prefer that the failure flag not mention ESC: just MOTOR_FAILURE_OVERTEMP That would generalise for motors not driven by an ESC, and removes the scope creep from this particular PR. Regards, James

…

On 11 Aug 2021, at 5:06 pm, Hamish Willee ***@***.***> wrote:  To maybe address @auturgy observation, I could define the temperature as for other MAVLink types where the field is populated with 0 if motor_temperature is not provided, and fill with 0.01C if the motor is actually at 0C. This should enable message trimming, thoughts? That would actually not allow trimming unless the motor_temperature was indeed an extension field - because the ordering of the fields pushes smallest ones last, and these are what get trimmed. And we don't want it to be an extension field because extension fields are the only ones checked for compatibility. Also because as soon as you further extend the trimming doesn't happen. An (undesirable) option would be a configuration field that states the temperature is motor or ESC. So on occasion you could send this with motor temp <field type="uint8_t" name="temp_type" instance="true">If 0, the temperature is ESC temp, if 1, it is motor temp.</field> Or another message for the motor temperature, when you discover that you actually need it. As suggested by @auturgy @auturgy This is the low rate message, which is already quite large. I don't argue that we generally don't want bigger messages, but is this really such an issue? PS: also added ESC_FAILURE_MOTOR_OVER_TEMPERATURE to ESC_FAILURE_FLAGS Excellent idea whatever else we do here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[hamishwillee]

@cmic0 Let's motor temperature be removed to a separate message then.

@auturgy Not sure about MOTOR_FAILURE_OVERTEMP. If we move motor temperature out then failures in the motor should move out too. Arguably you could keep ESC_FAILURE_MOTOR_OVER_TEMPERATURE though to reflect it is an ESC failure cause by motor failure [don't know if this is a useful distinction].

[auturgy]

I haven’t dug into those flags, but motors and ESC’s are different things - if they’re being conflated, it’s wrong.

…

On 12 Aug 2021, at 8:27 am, Hamish Willee ***@***.***> wrote:  @cmic0 Let's motor temperature be removed to a separate message then. @auturgy Not sure about MOTOR_FAILURE_OVERTEMP. If we move motor temperature out then failures in the motor should move out too. Arguably you could keep ESC_FAILURE_MOTOR_OVER_TEMPERATURE though to reflect it is an ESC failure cause by motor failure [don't know if this is a useful distinction]. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[hamishwillee]

I haven’t dug into those flags, but motors and ESC’s are different things - if they’re being conflated, it’s wrong.

@auturgy The other flags are fine.

My argument was that the proposal to rename the new flag from ESC_FAILURE_MOTOR_OVER_TEMPERATURE to MOTOR_FAILURE_OVERTEMP conflates the ESC message (so lets not do that).

The implied question is "does ESC_FAILURE_MOTOR_OVER_TEMPERATURE" also conflate them. I think it does, so probably the best thing is as you have proposed create a new message that covers motor faults - and here I am assuming we're talking electric motors controlled by ESC, not motors "generally".

[hamishwillee]

As discussed in dev call with @cmic0 .

The plan is to

• reduce ESC_INFO to be ESC specific - i.e. remove the motor temp and any motor-specific faults.
• add new "generic" MOTOR_INFO message. Capture as much as is useful generically about motors and motor faults in this. Consider the other messages like EFI_STATUS for inspiration.
• Both messages WIP. Suggest doing the MOTOR_INFO separately because we can then we can merge the ESC_INFO to address current need.

The argument for this approach is future proofing. ESC used extensively now, but approach might change. Generic motor message might be sufficient for many users and we can perhaps in future layer specific motor types on top.

Hope that matches your recollection Claudio!

[cmic0]

Hope that matches your recollection Claudio!

Perfectly resumed @hamishwillee :-), you were faster in typing the summary!
I'll do the changes today

[cmic0]

@RicardoM17 i've opened the PR on PX4 side to align with those changes, let me know if you want to go ahead in making the PR for mavros or if I should do it 👍

[RicardoM17]

@RicardoM17 i've opened the PR on PX4 side to align with those changes, let me know if you want to go ahead in making the PR for mavros or if I should do it +1

Sounds good Claudio. I can take care of the changes in mavros 💪

[hamishwillee]

Thanks @cmic0 and @RicardoM17 . This is what was agreed in dev call so I will merge. FYI @auturgy

Note @cmic0 hoping to see that follow up motor information message at some point! FYI As discussed, we are happy not to remove this WIP at this point, but we will want to sometime.

[cmic0]

Note @cmic0 hoping to see that follow up motor information message at some point! FYI As discussed, we are happy not to remove this WIP at this point, but we will want to sometime.

It's on the list, if i recall correctly i even have the branch ready, got probably sidetracked before pushing to create a PR for discussing it :-)!
Will try to do it this week

Thanks for the support @hamishwillee @auturgy @RicardoM17!