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 odd binding behaviour #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pkendall64
Copy link
Contributor

There is a weird bind behaviour on the TX backpack.

Problem

The TX backpack on receiving a bind message from the TX module, stores the UID and forwards the bind message, via ESPnow, to any listening VRx backpacks. The problem is that if the TX backpack is flashed with a passphrase it will always use the flashed UID as the ESPnow group address. So the upshot is that the VRx has bound to the TX address, not the TX backpack address.

Solution

When sending a bind from the TX backpack, if the backpack was flashed with a passphrase, overwrite the address in the message with the flashed address.

@wvarty
Copy link
Contributor

wvarty commented Feb 12, 2023

I actually think the "issue" you described is what we want (i.e. working as expected).
The same bind phrase is supposed to be used across the ELRS ecosystem in order to bind, so there should not be a case where the TX bind is different to the TX backpack bind, and if it is different, then it should not communicate.

@pkendall64
Copy link
Contributor Author

Yeah, I know what you're saying, so it would be better to have the main firmware tell the TX backpack what the bind UID is rather than configure it at build time. That way it's always correct.

@wvarty
Copy link
Contributor

wvarty commented Feb 12, 2023

Yeah, I know what you're saying, so it would be better to have the main firmware tell the TX backpack what the bind UID is rather than configure it at build time. That way it's always correct.

We could do that I suppose.
It makes things a little confusing, since it would be the one firmware that you DON'T need to supply your binding phrase on build, which might catch people out a little, but it solves the problem.

@wvarty
Copy link
Contributor

wvarty commented Feb 12, 2023

An easier approach is to prevent the TX-backpack from sending any messages if it has a different UID to the TX

@pkendall64
Copy link
Contributor Author

An easier approach is to prevent the TX-backpack from sending any messages if it has a different UID to the TX

I don't like the idea that it just silently doesn't work. Users will/have been confused as to why they can bind but it doesn't seem to work. With the code that returns the version number we could send back the UID in the backpack and then in the LUA it could say "UID mismatch"

@wvarty
Copy link
Contributor

wvarty commented Feb 12, 2023

An easier approach is to prevent the TX-backpack from sending any messages if it has a different UID to the TX

I don't like the idea that it just silently doesn't work. Users will/have been confused as to why they can bind but it doesn't seem to work. With the code that returns the version number we could send back the UID in the backpack and then in the LUA it could say "UID mismatch"

Doesn't it "silently fail" right now if you flash a TX and RX with different phrases?
In saying that, I'm fine with either of the approaches you've suggested. Moar work is all.

@pkendall64
Copy link
Contributor Author

An easier approach is to prevent the TX-backpack from sending any messages if it has a different UID to the TX

I don't like the idea that it just silently doesn't work. Users will/have been confused as to why they can bind but it doesn't seem to work. With the code that returns the version number we could send back the UID in the backpack and then in the LUA it could say "UID mismatch"

Doesn't it "silently fail" right now if you flash a TX and RX with different phrases? In saying that, I'm fine with either of the approaches you've suggested. Moar work is all.

TX and RX won't connect, but backpack to backpack it will bind using the bind-phrase from the TX module (passed in the bind command) but then it tries to talk with the UID on the backpack.

I noticed it when I added bind in the goggles and I put a success on the display when it gets a bind message. Then it didn;t get any messages because I fat-fingered the passphrase on my TX module so it was different to the TX backpack. Took me all day to figure out what had gone wrong!

@wvarty
Copy link
Contributor

wvarty commented Feb 12, 2023

An easier approach is to prevent the TX-backpack from sending any messages if it has a different UID to the TX

I don't like the idea that it just silently doesn't work. Users will/have been confused as to why they can bind but it doesn't seem to work. With the code that returns the version number we could send back the UID in the backpack and then in the LUA it could say "UID mismatch"

Doesn't it "silently fail" right now if you flash a TX and RX with different phrases? In saying that, I'm fine with either of the approaches you've suggested. Moar work is all.

TX and RX won't connect, but backpack to backpack it will bind using the bind-phrase from the TX module (passed in the bind command) but then it tries to talk with the UID on the backpack.

I noticed it when I added bind in the goggles and I put a success on the display when it gets a bind message. Then it didn;t get any messages because I fat-fingered the passphrase on my TX module so it was different to the TX backpack. Took me all day to figure out what had gone wrong!

Nah my suggestion was to prevent ANY comms if the passphrases dont match. Even the bind message.

@pkendall64
Copy link
Contributor Author

Nah my suggestion was to prevent ANY comms if the passphrases dont match. Even the bind message.

I'm cool with that. We will have to make people aware that if it doesn't bind then mismatching UIDs could be the reason.

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.

None yet

2 participants