Revert "Mavgen C: Use strncpy instead of memcpy to pack char[n] fields"#1143
Revert "Mavgen C: Use strncpy instead of memcpy to pack char[n] fields"#1143peterbarker merged 2 commits intoArduPilot:masterfrom
Conversation
This reverts commit ff57508. The change broke the param_value field of PARAM_EXT_... messages which is defined as char[128] and can contain arbitrary data such as int/float or binary data. Ideally, the type would be defined as uint8_t[128] and the issue avoided, however, this can't be changed at this stage as it would change the CRC_EXTRA and make the message incompatible. Unfortunately, I don't see another way to fix this than to revert this change.
|
@DonLakeFlyer this might also be relevant to #1142 |
|
@julianoes I'd like to see a second patch in this PR adding a comment as to why we're using memcpy - so we don't break your toys again. |
We tried to use strncpy in the past but it broke things.
|
I'm definitely leaning towards the "unbreak things by merging this". But I'd also like @shancock884 to comment first as his patches were trying to fix real problems. Perhaps we need to add another API which passes in the maximum number of bytes of data which should be copied into the passed-in buffer? That parameter missing is just a sore thumb as it is. |
|
Hmmm, ok. So if I understand right, char datatype has being used for a field which is really a binary blob. In the short term 4/ seems like the 'least worst' option to fix it. (1/ is nasty, and 2/ can take years!). |
|
Thanks @shancock884 - sadly I think we do need to go with option 4 as we've seriously broken the PX4_EXT stuff. Merging this one, thanks @julianoes |
|
1/ We really can't do that, as it causes breakage, unfortunately. |
|
Thanks @peterbarker for merging. |
This reverts commit ff57508 from #922.
The change broke the
param_valuefield ofPARAM_EXT_...messages which is defined aschar[128]and can contain arbitrary data such as int/float or binary data.Ideally, the type would be defined as
uint8_t[128]and the issue avoided, however, this can't be changed at this stage as it would change the CRC_EXTRA and make the message incompatible.Unfortunately, I don't see another way to fix this than to revert this change.
@shancock884, @hamishwillee