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

OTX 2.2 RC11 + BF 3.1.6 --> PID over Taranis lua script not working anymore #4560

Closed
RipperDrone opened this issue Mar 5, 2017 · 50 comments

Comments

Projects
None yet
@RipperDrone
Copy link

commented Mar 5, 2017

Seems like the current BF 3.1.6 lua scripts don't go along with the new release of OpenTX 2.2 RC11.
No PID values shown anymore (just "---") with the latest OpenTX and BetaFlight lua script build - flashing the Taranis x9D+ back to OpenTX RC10 works fine, PID values shown.

There have been changes in the lua script engine / syntax / float number formats as I read from the Wiki. This might have to do with a new mismatch btw OTX and BF lua scripts.

Does this bug have to be fixed on the OpenTX side or on BetaFlight side?

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

As mentioned on the release page, there may be issues with precompiled scripts. Delete any *.luac if you have some.

@RipperDrone RipperDrone changed the title OTX 2.2 RC11 + BF 3.1.6 --> lua script not working anymore OTX 2.2 RC11 + BF 3.1.6 --> PID over Taranis lua script not working anymore Mar 5, 2017

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 5, 2017

You mean on the Taranis? Will check all files, but in the scripts sub folder, there is no .luac file whatsoever...

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 5, 2017

Checked it all, no *.luac. I only have the 2 files

msp_sp.lua and ui.lua sitting in the SCRIPTS/BF/ folder and
BF.lua sitting in the SCRIPTS/TELEMETRY/ folder...

@3djc

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2017

We did not author those scripts, so it is really hard for us to guess what is wrong. It's really up to the script author to check them and report if there is an issue on our side, or adapt the scripts if it is the result of one of the documented changes

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 5, 2017

Ok, then I will report this bug to BF opentx lua scripts branch issues section, hoping that the author can identify where the problem lies...

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

We have verified our own scripts that use the same mechanism for FrSky S6R configuration and they run fine, so not sure

@projectkk2glider

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

My first suspicion here is DIY ID range change. The script would not run at all if it was a double/float problem.

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 5, 2017

Replicated same bug on 2 different FCs now, PikoBLX and BlueJayF4. What should I do next to help diagnosing?

Have you tested with BF 3.1.6 and OpenTX 2.2RC11 on Taranis X9D+?

@projectkk2glider

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

A link to script please.

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

@projectkk2glider

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

From the script it is not evident which dataId is used when sending the MSP frames. Somebody should check in betaflight source. The valid range now is:

#define DIY_STREAM_FIRST_ID       0x5000
#define DIY_STREAM_LAST_ID        0x50ff

See d594843

@lshems

This comment has been minimized.

Copy link

commented Mar 5, 2017

Just for info: if you go from 32 bits integer to 64 bits float, there is no problem. All HEX presentations of 32 bit values are correct. This is the standard discussion about integers and float, there not being a difference.

But.......

If you go from 32 bits integer to 32 bits float, all gets messed up. Try this:

print("0x7a09e66b",string.format("%x,%X",0x7a09e66b,0x7a09e66b))
result on 2.1, 32 bit Integer: 0x7a09e66b 7a09e66b, 7A09E66B
result on 2.2, 32 bit Floating Point: 0x7a09e66b 7a09e680 7A09E680

Then the bit32 library becomes useless as well.

Just my cup of tea.

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 5, 2017

There is another user having the exact same issue. When going back to OpenTX 2.2 RC10, it all works. Thus, something must have changed recently...

betaflight/betaflight-tx-lua-scripts#21

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

Yes we know it's not working. But we don't see why it wouldn't at this point, apart from the IDs having changed which Betaflight would need to follow

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 5, 2017

Ok, asking BF team for range of dataID next...

@mpaperno

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

@lshems is correct -- long ints (> 20 24 bits) are losing precision because Lua is converting them to floats and then back again (by simple casting, not bitwise). This is definitely a change from RC10 due to Lua using floats instead of doubles now. I'm looking for possible solutions (w/out going back to doubles).

I don't know if this is related to this specific issue, but I do see some bitwise operations taking place in the script... anything over 20 24 bits will very likely lose precision (change value) in the int->float->int conversion.

@raphaelcoeffic

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Gentlemen, the script does not need the dataID being in that range (especially not for sending). When betaflight sends the packets to the script, it uses primId = 0x32, so that the packets gets passed directly, without caring about the DIY_STREAM_FIRST_ID & DIY_STREAM_LAST_ID. That allows us to use more bit-bandwidth, which would be otherwise even poorer as it is now.

Please look at https://github.com/opentx/opentx/blob/next/radio/src/telemetry/frsky_sport.cpp#L234 for more details.

@raphaelcoeffic

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Now, that being said, I'll have a look into what could have broken it in RC11.

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

I have tried to work backwards in the commit history last night... but I did so on my X7, and it still wasn't working when I reached RC10.
Hadn't tested it previously, so it probably never worked, lost some time assuming it would.

@mpaperno

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

My Lua scripts which rely on 32-bit ints (for bitfields) also fail with RC11 (no errors in my case, "just" wrong data). Doubles can precisely represent ints up to 53 bits (vs. 24b for floats), so this wasn't a problem with previous versions.

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Just worked my way up on X9 and indeed the BF script breaks on a1a58ad

@3djc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

It makes perfect sense, he packs several bytes into a 32int, but when trying to unpack it from the script, it converts to float and looses acccuracy

@raphaelcoeffic

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

@3djc that's right, the values in the SPORT packet need to be transmitted to LUA without a single bit lost. Otherwise, it does not make any sense to transmit these bits over SPORT in the first place.

@RealTadango

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

Strange, i pack 4 bytes into one 32 bit value also and it still works fine. I transfer string text so it would be easy to spot it some bits got lost :)

This should still work as long as the address is 0x5000 to 0x50ff.

@3djc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

I think it may well have to do with how the are unpacked

@raphaelcoeffic

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

@RealTadango do you transmit only one way? (to a LUA script), or the other way as well?

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Address is indeed only used for telemetry, not for configuration as in the BF/S6 scripts

@RealTadango

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

Never mind, i send only lower values....... that are untouched.

@FPWibi

This comment has been minimized.

Copy link

commented Mar 6, 2017

The discussed script can change the PID Settings but also the VTX Settings. Setting the VTX Channels is still working with RC11. Only PID is not working.

@3djc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

There is no need to keep adding info in this thread, we have a clear understanding on what is happening, and are discussing ways to fix it.

Raphael could probably come with a way to temporarily avoid the issue, but that hardly qualifies as a fix.

@projectkk2glider

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

OK here is the plan:

A quick fix for this will be: going back to double for LUA_NUMBER. I will take care of it.

Long term solution is still under the discussion, here are the possibilities:

  • upgrade to Lua 5.3 which has native support for integers. Downsides: need to port our rotable #1645 and other patches to the new Lua version, incompatibilities with 5.2 https://www.lua.org/manual/5.3/manual.html#8)
  • Use LNUM patch http://luaforge.net/projects/lnum/. Problems: need to port this patch to Lua 5.2.2.
  • Do nothing - use double as before. Downsides: slower execution of Lua arithmetics, especially on the radios that have hardware FPU (which only supports float).

@projectkk2glider projectkk2glider self-assigned this Mar 6, 2017

@RealTadango

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

Upgrading would be a nice solution because i have a need for newer io options also, but it might also mean my scripts need re-work (not sure, but they probably need some changes). Still i think upgrading to 5.3 is the best solution for the future and now might be a good time.

@3djc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

The be more specific on timeline, : 2.2.0 will be with the quick fix (back to double), longer term will be for 2.2.x (where x shouldn't need int32 to store ;))

@raphaelcoeffic

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Thx gentlemen!

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 6, 2017

@3djc @projectkk2glider Will you re-issue the 2.2.0 nightly build somewhere inclusing the issue #4560 patch / is there an automated Jenkins build compiling the source code into target files somewhere?

I don't have the build environment installed, therefore having a Jenkins type download option would be very appreciated...

@3djc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

We do not have a nightly building mechanism available right now for 2.2. It will be available once we go with release. The fixes will be available in next rc. We understand the issue, and how it matter to you , you need to understand that releasing an rc is no small thing, and impact a lot of people

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 6, 2017

Ok, thank you anyway... will revert back to RC10 then, best option for now...

@projectkk2glider

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Test firmwares (English language, USB Joystick, 3 Timers, Lua):

@mpaperno

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Most definitely fixes the issue of 32 bit int representation and other anomalies which accompanied it (there were quite a few!).

@FPWibi

This comment has been minimized.

Copy link

commented Mar 6, 2017

Setting the PIDs is working again. Thank you!

@Corruptsector

This comment has been minimized.

Copy link

commented Mar 7, 2017

Just Setup my Taranis with the above FW and can also confirm working for LUA script for both PIDs and Unify.

@RipperDrone

This comment has been minimized.

Copy link
Author

commented Mar 7, 2017

Thank you, Taranis OTX 2.2 greatness is back 😁

@1Sven

This comment has been minimized.

Copy link

commented Mar 7, 2017

Is there a fix-version for Horus available? I can’t do it self…

@jyzoom

This comment has been minimized.

Copy link

commented Mar 7, 2017

Any Possibility of a test firmware for Qx7?

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

For me the X7 script in the betaflight repo doesn't work even with the fix (and doesn't with N362 either). I think the script has an issue. @raphaelcoeffic ?

@AABatteries

This comment has been minimized.

Copy link

commented Mar 7, 2017

@kilrah script for X7 (v0.3) works with no issues when launched via SD card browser on N632. However it does not work when set up as a custom telemetry screen on N632

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

There are several X7 scripts that different people have made based on the X9 one. I'm talking of the "official" one here: https://github.com/betaflight/betaflight-tx-lua-scripts

Might want to mention where you sourced yours from

@AABatteries

This comment has been minimized.

Copy link

commented Mar 7, 2017

@kilrah Yes, I've been using the very one that you linked, and it does indeed work. It glitches out when used as a telemetry screen, but works when launched via the SD browser. I've confirmed that the settings are getting saved as well

@Rups63

This comment has been minimized.

Copy link

commented Mar 8, 2017

I can second that. For RC10, on the X7, the script from https://github.com/betaflight/betaflight-tx-lua-scripts v0.3 does work when launched from the SD card menu, but not when used as a telemetry screen. Now on RC11, it does not work when launched from the SD card menu or when used as a telemetry screen.

@kilrah

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

OK, I'll have to try and copy them again, for some reason they don't work here even launched from browser (only tried that anyway)..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.