Use named float wrapper to fix GCC 8.1 compile error

[WickedShell]

Fixes #8353 with a silly 10 byte name. Required to let GCC 8.1 work.

[magicrub]

Question, does C do the usual thing of appending a null char to that string? If so, does that make it 11 chars?

I imagine if you took a couple off then gcc gets mad, but does it also get mad if you have too many?

[patrickelectric]

It was not possible to compile with this patch, did you tried it ?

[WickedShell]

@patrickelectric it compiles for me with GCC 8.1, what compiler are you using, and whats the error? (Also compiles under all of CI correctly)

[WickedShell]

@magicrub your right, it was appending a character, I've removed an unneeded one, and it doesn't care if there are to many. The complaint was memcpy'ing out of bounds from the string, if the string is larger then we copy it doesn't care.

[patrickelectric]

This same function is used in other places around the code.

ArduCopter/mode_flowhold.cpp:    mavlink_msg_named_value_float_send(MAVLINK_COMM_0, AP_HAL::millis(), "HEST", height_estimate);
ArduCopter/mode_flowhold.cpp:    mavlink_msg_named_value_float_send(MAVLINK_COMM_1, AP_HAL::millis(), "HEST", height_estimate);
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(
ArduSub/GCS_Mavlink.cpp:    mavlink_msg_named_value_float_send(

Compiler:

gcc (GCC) 8.1.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Output: https://paste.kde.org/pzlcyl37c

[WickedShell]

@patrickelectric Oh, that's the same class of issue, just affecting the other vehicles. (I was testing Plane here at the time), those vehicles would also need similar changes to resolve them.

[tridge]

I think a wrapper function in the GCS_MAVLink library would be a better soln for this

[WickedShell]

Added a wrapper, if everyone (@peterbarker @OXINARF @tridge ) is good with this approach I will fix the sub/copter versions of this tomorrow and push them to this PR.

[peterbarker]

LGTM.

Noting, however, that it's a significant behavioural change, sending to all active channels rather than just a special, magic one.

[WickedShell]

Yes, but the magic one is something I'd call won't for airspeed.... Heck I almost just used it as an excuse to cut the message until we get a proper one :)

…

On Tue, May 15, 2018, 00:23 Peter Barker ***@***.***> wrote: LGTM. Noting, however, that it's a significant behavioural change, sending to all active channels rather than just a special, magic one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAipiHfmJuhCbUdn4d1gIGSJbKGuOnQPks5tyoKDgaJpZM4T8uqJ> .

[WickedShell]

s/won't/wrong

…

On Tue, May 15, 2018, 00:23 Peter Barker ***@***.***> wrote: LGTM. Noting, however, that it's a significant behavioural change, sending to all active channels rather than just a special, magic one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAipiHfmJuhCbUdn4d1gIGSJbKGuOnQPks5tyoKDgaJpZM4T8uqJ> .

[OXINARF]

@WickedShell I was expecting changes to all the other places where this is used too. That was the purpose of adding a wrapper 😉

[OXINARF]

Why the +1? The char array needs exactly MAVLINK_MSG_NAMED_VALUE_FLOAT_FIELD_NAME_LEN. Contrary to vsnprintf, strncpy will copy every character until it can, it doesn't reserve space for null-terminator.

[WickedShell]

Because if you don't do that, with GCC 8.1 you have to deal with the following warning:

../../libraries/GCS_MAVLink/GCS_Common.cpp: In member function ‘void GCS_MAVLINK::send_named_float(const char*, float) const’:
../../libraries/GCS_MAVLink/GCS_Common.cpp:1532:12: warning: ‘char* strncpy(char*, const char*, size_t)’ specified bound 10 equals destination size [-Wstringop-truncation]
     strncpy(float_name, name, MAVLINK_MSG_NAMED_VALUE_FLOAT_FIELD_NAME_LEN);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm flooded with these for statustexts, and was just trying to not add an additional one. (But I agree it's not actually needed for MAVLink). If we want to turn off the warning then I can see removing it...

[OXINARF]

GCC 8.1 is killing me. Is it a compiler or a teaching tool? Gosh.

Anyway, rather than disabling the warning I'd prefer to use the strategy that GCC docs suggest (https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html): To avoid the warning when the result is not expected to be NUL-terminated, call memcpy instead.

The incoming string has to be NUL-terminated, so doing a memcpy should always work, it either sends a NUL-terminated string smaller than the array size or it sends a not NUL-terminated string truncated to the array size.

[OXINARF]

Again, why? This array position won't be read/copied by the MAVLink code.

[WickedShell]

Copter and Sub are both wrapped now, copter now sends to all active channels as well. I ran the behavior changes past tridge and randy on mumble and got affirmative opinions on the behavior change of sending to all channels as being correct.

[OXINARF]

Merged, thanks!