Updated screens to support u16 version of dtermSetpointWeight

[codecae]

Corrects issue where dtermSetpointWeight cannot be updated due to data type change in the MSP definition in 3.4.

[mikeller]

Weird that this is causing problems, the MSP implementation in the firmware should be using byte 10 as the value (only up to 254 obviously) if bytes 24 / 25 are not present. Is the lua script sending too many bytes? If so, this will potentially cause breakage in many other places in the future.

[codecae]

The script receives the entire MSP frame and stores it in memory. Since the frame now contains two different versions of dterm setpoint weight (8 bit and 16 bit), it stores them both and sends them both back. Since the entire frame is returned, the 8 bit version is applied and subsequently the 16 bit version is applied on top. If the script only updates byte 10 and not bytes 24&25, the change applied by byte 10 is reverted.

[codecae]

IMHO, having two interpretations of the same attribute that are applied in sequence by the FC seems a bit kludgy. If this is required, then the FC should not only look at the number of bytes but also create a guard that prevents the 16bit value from updating dtermSetpointWeight if the 8bit has already changed it.

[mikeller]

@codecae:

IMHO, having two interpretations of the same attribute that are applied in sequence by the FC seems a bit kludgy. If this is required, then the FC should not only look at the number of bytes but also create a guard that prevents the 16bit value from updating dtermSetpointWeight if the 8bit has already changed it.

'Append new values at the end' is how MSP is designed to handle backwards compatibility. If the lua scripts don't do this properly, then that should probably be fixed.

Forcing all clients to send the lower 8 bit of the previous value in the legacy field just to stop the firmware from reverting to the legacy behaviour and limit the value range for this parameter sounds like a very cumbersome approach to working around this shortcoming on the client.

[mikeller]

This will break backwards compatibility with older versions of the firmware. The minimum length for supported versions of the firmware is still 23 bytes.

[mikeller]

This should be kept in, to support older versions of the firmware.

[codecae]

@mikeller totally understand the 'append new values at the end' concept. Reducing minBytes to 23 is fine to keep support for older versions. The script will handle new bytes gracefully as they are added, so no problem there either. This situation, though, is unique because the same call is setting the same variable twice on the FC side. The FC should skip over setting it the second time if the first update already made a change.

Also, say for example something new is added to MSP_SET_PID_ADVANCED that isn't already in the list, after the second setter for dtermSetpointWeight. In order to send the bytes following the second dtermSetpointWeight value, it too will also need to be sent back to the FC. The FC could handle this exception much more gracefully than the script.

[codecae]

Otherwise, we will have to impose a maxBytes configuration which will prevent extensibility beyond 23 bytes.

[mikeller]

@codecae:

This call is by no means unique, this pattern is used in a number of other places, and it works well for all other configuration applications.

The FC could handle this exception much more gracefully than the script.

The flight controller handles this situation (there is no exception) exactly in the way that MSP was designed to work - it supports old applications sending only the first value, and new applications filling an arbitrary value into the slot for the first value, and sending the second value.

I do not see what the problem is with what you describe - this is exactly how it is intended to be used - applications that update their support level to a newer version will have to do so by supporting all of the changes in between. Doing a 'pick and choose' of what changes they support won't work, but I don't think there is any expectation of that.

Also, please note that blindly resending the frame that was received from the getter as parameters to the setter is only working by pure chance - there is no requirement that the frame formats are identical, and this isn't always the case.

[codecae]

So what is the proposition? Hard stop sending bytes at 23?

[codecae]

Btw, I was saying unique with respect to the scripts, not the protocol in general.

[mikeller]

My suggestion would be:

1. leave minBytes at 23;
2. if more than 23 bytes are received, use 24, 25 for dtermSetpointWeight, 10 otherwise;
3. set byte 10 to MIN(dtermSetpointWeight, 254);
4. set 24, 25 to dtermSetpointWeight.

(4. can be made conditional to more than 23 bytes being received, but if that's too much cost in terms of script size it doesn't - the protocol is designed to gracefully ignore any bytes bejond the ones it knows about.)

In general, I think it would be a an improvement over the current implementation to store outputBytes for every command to hold how many bytes the known frame is, and only send that many bytes back, even if the frame received from the getter was longer. This avoids sending back data that we don't know about.

[codecae]

I updated it with a simple truncation of the output values to reduce the returned bytes to a max of 23.. think this will work okay?

[codecae]

I'll take a look at seeing if those conditions can be applied, but it might be a little difficult as the screen is drawn as the reply is processed.