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

Add support for extended CRSF RF Modes #39

Merged
merged 4 commits into from
Apr 13, 2022

Conversation

FrankPetrilli
Copy link
Contributor

@FrankPetrilli FrankPetrilli commented Apr 4, 2022

When using ExpressLRS, the packet rate in the top-right of the telemetry widget is inaccurate, and shows 50Hz while operating at packet rates with RF Mode > 2. This change introduces a new configuration item, which when set to "ExpressLRS", supports RFModes up to 8 (1000Hz), and correctly displays rates for ExpressLRS systems.

Translations are still needed, but otherwise is functional.

@wx4cb
Copy link

wx4cb commented Apr 4, 2022

couldn't you just make the assumption that anything over 2 is going to be ELRS? saves user error... also, what happens if it's set to ELRS but the user is using TBS?

@FrankPetrilli
Copy link
Contributor Author

Good question! RFMode 1/2 have different interpretations for Crossfire & ExpressLRS - 1/2 are 50/150Hz vs 25/50Hz respectively. If a user has the config set to ELRS and the user is using Crossfire, they get 25/50 displayed instead of 50/150.

@wx4cb
Copy link

wx4cb commented Apr 4, 2022

Good question! RFMode 1/2 have different interpretations for Crossfire & ExpressLRS - 1/2 are 50/150Hz vs 25/50Hz respectively. If a user has the config set to ELRS and the user is using Crossfire, they get 25/50 displayed instead of 50/150.

i see what you mean, i wonder if there's any way you could read the board type via lua, kinda like the agent-lite lua does, should only need to be done once on bootup. would have been nice if the elrs guys matched the current crossfire spec, but hey, i'm not going to get into politics here lol

@FrankPetrilli
Copy link
Contributor Author

Yeah, I was thinking the same - it'd require some significant work and code outside of the existing framework of the widget (telemetry value focused). The ExpressLRS LUA does this, but I didn't want to break too far out of the mold / design.

@wx4cb
Copy link

wx4cb commented Apr 4, 2022

Yeah, I was thinking the same - it'd require some significant work and code outside of the existing framework of the widget (telemetry value focused). The ExpressLRS LUA does this, but I didn't want to break too far out of the mold / design.

well one thing you COULD do is maybe call the elrs script functions in the inav lua maybe? I know it's getting it more complicated, and a simple config value would fix it simply. but i was just trying to head off someone saying "hey my lua says on 150 hz but i'm running elrs at 500... what gives" LOL

@FrankPetrilli
Copy link
Contributor Author

I was able to make this work - sorta. I'm wrestling with how to get the check to work reliably on boot since crossfireTelemetryPop(), isn't blocking, and finding where to nestle this thing in there so that it only runs once things are up is proving to be a bit of a challenge. I'm stepping away from it for the evening, but will see if I can't make more progress soon.

local function elrs()
        -- enqueue a ping request
        crossfireTelemetryPush(0x28, { 0x00, 0xEA })
        local command, data
        i = 0
        -- give up after 50 tries
        while command ~= 0x29 and i < 50 do
                command, data = crossfireTelemetryPop()
                i = i + 1
        end
        if command == 0x29 then
                return parseDeviceInfoMessage(data)
        end
        return false
end

@FrankPetrilli
Copy link
Contributor Author

FrankPetrilli commented Apr 5, 2022

Late evening inspiration got me. This approach polls in background() until it's been identified one way or the other. Verified with my ELRS v2 and Crossfire setups, and works as expected.

There are a few more things I'd like to shore up before merging: handling error cases, validating non-CRSF behavior, etc.

@stronnag
Copy link
Collaborator

stronnag commented Apr 5, 2022

It is necessary to do the busy-wait each background() iteration?
Rather, would it be possible to set the check up crossfireTelemetryPush(0x28, { 0x00, 0xEA }) either during initialisation or in the first background call back, and then check crossfireTelemetryPop() once in subsequent backgrounds until you've exhausted a counter or had a response?
I appreciate this requires holding a bit more state (and I don't have the hardware to try it).

@FrankPetrilli
Copy link
Contributor Author

FrankPetrilli commented Apr 5, 2022

It is necessary to do the busy-wait each background() iteration? Rather, would it be possible to set the check up crossfireTelemetryPush(0x28, { 0x00, 0xEA }) either during initialisation or in the first background call back, and then check crossfireTelemetryPop() once in subsequent backgrounds until you've exhausted a counter or had a response? I appreciate this requires holding a bit more state (and I don't have the hardware to try it).

Thanks, good note! I've moved crossfireTelemetryPush() to being called only once - it works for me on Crossfire & ELRS, but it still needs testing with Multiprotocol, etc. It still polls without a timeout/failure count in the greater sense, but gives up after 50 reads on each iteration. If it gets either TBS or ELRS responses, it will not be called again, and there should be no cases where this gets called for non-CRSF modules.

@nm17
Copy link
Collaborator

nm17 commented Apr 5, 2022

Approved GitHub CI Checks, seems to be building just fine

@stronnag
Copy link
Collaborator

stronnag commented Apr 5, 2022

Thanks, good note! I've moved crossfireTelemetryPush() to being called only once - it works for me on Crossfire & ELRS, but it still needs testing with Multiprotocol, etc. It still polls without a timeout/failure count in the greater sense, but gives up after 50 reads on each iteration. If it gets either TBS or ELRS responses, it will not be called again, and there should be no cases where this gets called for non-CRSF modules.

I tested the previous commit on the Edge/2.7 simulator without any problems. I'll test current on TX16S/MPM tomorrow.

@stronnag
Copy link
Collaborator

stronnag commented Apr 6, 2022

And there are no issues on TX16S/MPM or simulator.

@FrankPetrilli
Copy link
Contributor Author

A new iteration - this one works reliably in all cases for me and reduces the number & location of retries down to the minimum that still works.

@stronnag stronnag merged commit 4cf3bcc into iNavFlight:master Apr 13, 2022
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