Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix YAML write issue with mix offset #1743

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Fix YAML write issue with mix offset #1743

merged 1 commit into from
Mar 28, 2022

Conversation

raphaelcoeffic
Copy link
Member

Fixes #1734

Summary of changes:

  • Mix data offset: convert to signed int with maximum 11 bits even if it uses 14 bits.

@raphaelcoeffic raphaelcoeffic added this to the 2.7 milestone Mar 27, 2022
@mha1
Copy link
Contributor

mha1 commented Mar 27, 2022

@raphaelcoeffic Thank you very much.

I merged this PR in a private build. This fixes the "offest with GV's" problem I demonstrated in GV1YAML1.otx.zip and with my real 2.3.14 model .bin files. I didn't notice anything else out of the ordinary but mind you this was very limited testing. So I can't say if this PR breaks anything else. I hope YAML conversion gets more and broader re-testing.

Michael

@raphaelcoeffic
Copy link
Member Author

@mha1 Thx Michael, we will have some additional tests. @pfeerick ?

@mha1
Copy link
Contributor

mha1 commented Mar 28, 2022

@raphaelcoeffic - Hi Raphael, a nearly totally unrelated question. Why does destCH counting start at 0? My guess is one reason for switching to YAML was to have model definitions in a human readable format. Looking through the .yml files I found myself getting confused with destCH over and over again. Not that I am not used to indexes starting at 0 but in this case: the channel numbering convention for human users start at 1. Not easy to keep this in mind looking through .yml files not to speak of the mental trap you are running in editing .yml by hand. I think same goes for Inputs Ix.

@pfeerick
Copy link
Member

This would be the unfortunate "programmer vs human" dilemma... i.e. counting always starts from 0, not 1.

Being human readable was certainly one reason for the change to yaml, but the underlying reason was to have it something that was not locked in a binary structure file... ie. the config format itself now does not place any limiations on the sizes of fields... if we want to make it so you can have 30 character model names... we can do that now... without it crashing into the next field in the file.

So whether the indexing is changed in a future version I don't know... it depends how far we want to go with making it "human readable"... as if you wanted to start on that aspect, there are a lot more settings that are currently complete gibberish to interpret unless you look at the code... But, IMO, at the end of the day, we don't really want you to be looking at the yaml... that's for the radio or some companion software... if you're looking at it, you're either a programmer working on the firmware, or a companion tool of some sorts, or an advanced user. If you're not one of those... we've failed ;)

@mha1
Copy link
Contributor

mha1 commented Mar 28, 2022

Yeah, I get that. But human readable files are click bait for curious people like me :-) even if they shouldn't :-). And I fully understand the "we don't want you". Your honor, I rest my case.

Thanks for your time!

@pfeerick
Copy link
Member

pfeerick commented Mar 28, 2022

Your honor, I rest my case.

Ahem... what did I say... I forget now... it must have been good though! 😆

I'm afflicted by that same complaint though... if files are half-way intelligible.... the temptation to play around in them, and see what Easter eggs or features missing UI there are is just irresistible.

Anyway, as far as this PR addressing the main issue, It seems to be resolving it without any other side effects... I did a crazy model with all sorts of positive and negative offsets, and weights, -GV9/GV9, GV1/-GV1, (and opposites), basically anywhere I could see that accepted GV I shoved values in at both ends of range for inputs, mixes and outputs, and it seemed to behave itself... I did find out I had to use firmware version 2.5 to test one part of this (Mixer line, Curve Diff GV) as Companion 2.5 did not seem to be generating correct model bins for the radio - neither radio nor simulator liked curve diff with any GV value... mixer rows would show up empty... but was fine if created on radio - companion would read it, and firmware would correctly convert with this PR... companion 2.5 must have been broken for that particular setting.

@mha1
Copy link
Contributor

mha1 commented Mar 28, 2022

Thanks for testing. My worries were about the other places GV's are used. Great you covered this too.

My primary use case btw was "I'm migrating to Edgetx" using Opentx 2.3.14 generated .bin's.

@pfeerick pfeerick merged commit be9aa44 into main Mar 28, 2022
@pfeerick pfeerick deleted the otx-conv-gv branch March 28, 2022 23:25
@mha1
Copy link
Contributor

mha1 commented Mar 29, 2022

Thanks Raphael & Peter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yaml conversion of mixer with GV1 as offset leads to corrupt .yml file
3 participants