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

Modify FrSky D-series telemetry for compatibility with Lua script #3116

Merged
merged 10 commits into from Jun 22, 2018

Conversation

Projects
None yet
5 participants
@alexeystn
Copy link
Contributor

commented Apr 24, 2018

This PR makes old FrSky D-series telemetry receivers fully compatible with Lua Telemetry script by @teckel12.

Please, add 'In progress' label. I've run successful bench tests with D8R-II Plus receiver, but need to test it out in the field to make sure everything is ok.

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

The only difference with X-series is a distance sensor ID: X-series use 0420.
D-receivers cannot use 16-bit ID's, so I've chosen 07 which is not busy for any other purpose and can be transmitter to TX.

@fiam

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@alexeystn could we move the common code to a separate file? It looks like there’s quite some duplicated code in sport and d8 telemetry (I’m viewing on mobile, so I might be wrong)

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

@alexeystn Nice! I don't have a D-series Rx to test, but it seems solid. So the distance sensor on the transmitter will by default be named 07, which needs to be changed to Dist to work with LuaTelemetry. Is that correct?

@fiam Yes, there is a bit of duplication between smartport.c and frsky.c. How would you suggest this be dealt with in the smartest way? Should a smartport_shared.c, sport_shared.c, frsky_shared.c or flightmodes_shared.c file be created that would house this duplicated code? It seems that would be most appropriate. Wouldn't be much effort to go that route, but the most logical file naming convention would need to be nailed down.

@teckel12 teckel12 referenced this pull request Apr 24, 2018

Closed

Add support for FrSky D-series receivers #95

4 of 4 tasks complete
@fiam

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

I’d suggest we rename frsky.c to frsky_d8.c and we use frsky.c to put the common code. @digitalentity?

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

@teckel12 Yes, that is correct.
I have both D and X receivers in my iNav fleet, so I can check if they works equally.

@DzikuVx

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

I second the rename. It's very confusing

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

@fiam @DzikuVx Not that anything we do won't be confusing. But, making it frsky_d8.c is equally confusing because the D4R would also be compatible. Further, putting the unified S.Port and FrSky logic in a frsky.c file seems confusing as well, as historically we've referred to the D-series as FrSky and the X-series as S.Port.

Maybe frsky_d.c instead of frsky_d8.c?

@fiam

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

frsky_d.c seems like a better idea to me, but either way is good enough. I think we should stop using just brand names like just frsky because that's confusing (like e.g. "FrSky" in the telemetry combobox in the ports configurator tab).

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

@fiam 👍 For FrSky telemetry options it should be D-series, S.Port & F.Port. That makes far more sense now that there's 3 different FrSky telemetry options. But I'm sure that will cause just as much confusion with people that don't know that D-series is FrSky.

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2018

Today I have checked these changes with D8R-II-Plus in the field. Telemetry screen worked correctly.
Now file naming is the only question.

@teckel12
Copy link
Contributor

left a comment

I reviewed the D-series telemetry and didn't notice the dummy values, good catch!

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

@alexeystn Do you want to create the frsky_d.c common code file or should I take a stab at it? My only issue is testing, since I lack a D-series receiver.

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

@teckel12 That was my mistake, I left them after bench-testing inside the house without GPS accidentally.
Excuse me, I didn't understood our final decision from the foregoing discussion. Rename frsky.c with D-series telemetry to frsky_d.c and put common code to frsky.c, is it right? If so, I can do that.

@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

@alexeystn I believe that's right. frsky.c for the unified code, frsky_d.c for D-series specific, and smartport.c for the S.Port stuff. I can test S.Port, you can test D-series, and I'm sure someone can test F.Port, if that's touched at all.

@digitalentity digitalentity added this to the 2.0 milestone May 1, 2018

@teckel12
Copy link
Contributor

left a comment

Visual inspection seems correct. Will compile this branch and test in the next couple days.

@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 7, 2018

@alexeystn I also wanted to bring up the issue with the 16-bit variable overflow for the flight modes.

Do you happen to know if it's unsigned or signed? If it's unsigned there's probably an easy fix. But if it's signed, the D-series would never get a FAILSAFE notification.

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2018

@teckel12 I assigned Failsafe to the aux channel in configurator so that I could test it without real signal loss. When I toggle the switch, I see "Failsafe" notification on Taranis' screen. It means, unsigned 16-bit variable is sent correctly to D8R-II.
The only thing that concerns me is the fact that hypothetically value may exceed 65535, but it can be prevented with this fix.

@teckel12
Copy link
Contributor

left a comment

Still haven't tested the code, but I like the 16-bit overflow fix.

// todo: prevent 16-bit variable from overflow,
// D-series receivers transmit only 16-bit values
else
if (FLIGHT_MODE(AUTO_TUNE))

This comment has been minimized.

Copy link
@teckel12

teckel12 May 7, 2018

Contributor

@alexeystn Would this be more readable and more in line with the code standard to be

    else if (FLIGHT_MODE(AUTO_TUNE))
        tmpi += 20000;
// D-series receivers transmit only 16-bit values
else
if (FLIGHT_MODE(AUTO_TUNE))
tmpi += 20000;

This comment has been minimized.

Copy link
@teckel12

teckel12 May 7, 2018

Contributor

Maybe a note to clarify why it's being done this way (as a year from now someone will probably see this as a mistake and "correct" it). Something like this:

// intentionally reverse order and else if to prevent 16 bit overflow
@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

@alexeystn I compiled your branch last night and was planning on testing tonight, but something came up. Will test on Wednesday. Sorry for the delay.

@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@alexeystn Tested this PR on two quads today (SmartPort telemetry) and both worked without a problem.

@teckel12
Copy link
Contributor

left a comment

Works good with S.Port

@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@alexeystn For Lua Telemetry distance support I need to verify the exact distance sensor name. Delete the Dist sensor, search for sensors which will re-add the distance sensor. It was noted that the name was 07. I want to be sure it's exactly that, for example 0007 would be different as the value is a string not an integer.

I need to know this for two reasons. First, documentation. And secondly, so the script can use the default sensor name instead of needing to rename it (making it easier for users).

Thanks!

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

@teckel12
Sorry for my delay too. Tested it with D8R-II Plus on a flying wing, everything works good, including distance:
screen-2018-04-29-092300

Even though D8-receiver operates with 8-bit sensor IDs, Taranis displays it as 0007, not 07.
That's how it looks when I remove all sensors and discover them again:
screen-2018-05-09-235339

Since it is a string, not a number, it makes difference. You are right.
Good idea about automatic name detection!

@teckel12
Copy link
Contributor

left a comment

👍

@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@alexeystn Perfect! SPlissken has also agreed to test D-series receivers with Lua Telemetry.

@teckel12

This comment has been minimized.

Copy link
Contributor

commented May 11, 2018

@fiam @alexeystn I got a third confirmation from @SPlissken that this branch is functional with both X-series and D-series receivers. I'd say it's good to merge.

@fiam

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

Can you resolve the conflicts so we can merge it for 2.0?

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

Yes, I'll try to resolve them within a day or two.

@fiam

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

Thanks @alexeystn. Feel free to ping me after doing so and I'll merge right away.

alexeystn added some commits Jun 21, 2018

@alexeystn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@fiam I think it's done.

@fiam

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

Thanks @alexeystn! Merging.

@fiam fiam merged commit c0c2772 into iNavFlight:development Jun 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.