Skip to content

Added tz_automatic_dst to MSP2_COMMON_TZ and MSP2_COMMON_SET_TZ#6115

Merged
stronnag merged 2 commits intoiNavFlight:masterfrom
Airwide:aw-add-tz-to-fc_msp
Sep 17, 2020
Merged

Added tz_automatic_dst to MSP2_COMMON_TZ and MSP2_COMMON_SET_TZ#6115
stronnag merged 2 commits intoiNavFlight:masterfrom
Airwide:aw-add-tz-to-fc_msp

Conversation

@Airwide
Copy link
Contributor

@Airwide Airwide commented Sep 9, 2020

Added tz_automatic_dst to MSP2_COMMON_TZ and MSP2_COMMON_SET_TZ to be able to set time zone settings from inav-configurator.

@stronnag
Copy link
Collaborator

stronnag commented Sep 9, 2020

This breaks existing 3rd party applications that use MSP2_COMMON_TZ. The code should accept 2 or 3 bytes to maintain legacy compatibility.

@Airwide
Copy link
Contributor Author

Airwide commented Sep 9, 2020

This breaks existing 3rd party applications that use MSP2_COMMON_TZ. The code should accept 2 or 3 bytes to maintain legacy compatibility.

@stronnag Ok, I'll make changes to MSP2_COMMON_TZ. What about MSP2_COMMON_SET_TZ, is that ok?

@Airwide
Copy link
Contributor Author

Airwide commented Sep 9, 2020

@stronnag - Thinking a bit more about it, it's probably better if I create a new MSPCode just for this purpose (to communicate with INAV-configurator). WDYT?

@stronnag
Copy link
Collaborator

stronnag commented Sep 9, 2020

This breaks existing 3rd party applications that use MSP2_COMMON_TZ. The code should accept 2 or 3 bytes to maintain legacy compatibility.

@stronnag Ok, I'll make changes to MSP2_COMMONMSP2_COMMON_TZ. _TZ. What about MSP2_COMMON_SET_TZ, is that ok?

Apologies, sloppy review comment.

MSP2_COMMON_SET_TZ should accept two or three bytes, so legacy apps can just set the offset as before.

MSP2_COMMON_TZ doesn't need changing, any well designed app will read 3 bytes and process the two it expects. This is how we've handled similar cases where a structure has changed previously.

@Airwide
Copy link
Contributor Author

Airwide commented Sep 9, 2020

This breaks existing 3rd party applications that use MSP2_COMMON_TZ. The code should accept 2 or 3 bytes to maintain legacy compatibility.

@stronnag Ok, I'll make changes to MSP2_COMMONMSP2_COMMON_TZ. _TZ. What about MSP2_COMMON_SET_TZ, is that ok?

Apologies, sloppy review comment.

MSP2_COMMON_SET_TZ should accept two or three bytes, so legacy apps can just set the offset as before.

MSP2_COMMON_TZ doesn't need changing, any well designed app will read 3 bytes and process the two it expects. This is how we've handled similar cases where a structure has changed previously.

Ok, then it's quite easy to fix. Fix coming up!

@stronnag
Copy link
Collaborator

stronnag commented Sep 9, 2020

@stronnag - Thinking a bit more about it, it's probably better if I create a new MSPCode just for this purpose (to communicate with INAV-configurator). WDYT?

The above solution is fine. In general we'd prefer not to add new MSP Ids unless absolutely necessary.

Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@digitalentity digitalentity added this to the 2.6 milestone Sep 10, 2020
@Pairan
Copy link

Pairan commented Sep 11, 2020

Thanks @Airwide :)

@stronnag stronnag merged commit 0ba9160 into iNavFlight:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants