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

Wait for clear channel (not busy) when using VARA #387

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

bseidenberg
Copy link

Currently, Pat only implements the busy check for ARDOP. Do the same for VARA. The actual busy/not busy logic is already implemented in Pat-Vara/vara.go in handleCmd().

@bseidenberg
Copy link
Author

Note: I currently only have remote access to radios to test this, which makes generating the right RF rather difficult. But, it seems straight forward enough given the code.

connect.go Outdated Show resolved Hide resolved
@bseidenberg
Copy link
Author

Updated

@@ -152,7 +152,7 @@ func Connect(connectStr string) (success bool) {

// Wait for a clear channel
switch url.Scheme {
case MethodArdop:
case MethodArdop, MethodVaraFM, MethodVaraHF:
waitBusy(adTNC)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must also be changed. Passing adTNC here is only correct for ARDOP. We will need to pass a reference to the appropriate TNC connection, which depend on the URL scheme.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I got it. Sorry, i thought "adTNC" was the current one, I see now where the symbols come from.
Sorry, I'm still struggling with idiomatic Go, especially the typing system. (This is the only go code base I've touched).

And yes, to your earlier question, the VARA modem code does implement the Busy() method.

Will send try 3.

Currently, Pat only implements the busy check for ARDOP. Do the same for
VARA. The actual busy/not busy logic is already implemented in
Pat-Vara/vara.go in handleCmd().
Copy link
Member

@martinhpedersen martinhpedersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising 👍

The only thing remaining is to test this over some time, and under different conditions. Since we don't provide a method for overriding the "wait busy" state, we depend heavily on a good busy detector.

Do you have any chance of testing this soon, or organize some testing one way or another?

Thanks!

@bseidenberg
Copy link
Author

bseidenberg commented Feb 14, 2023 via email

@martinhpedersen
Copy link
Member

@bseidenberg - Did you get a change to test these changes as we discussed? 🙂

@bseidenberg
Copy link
Author

bseidenberg commented Apr 16, 2023 via email

@bseidenberg
Copy link
Author

OK, I think I understand what's going on...I think. All the other initModem/initTNC() functions are argument-less and directly manipulate the various variables in connect.go for their modems. However initVaraModem() takes a pointer to the modem. That pointer is passed by value as vModem. The vModem local is updated inside initVaraModem() to point to the new modem, but because it's passed by value, the outer "global" of varaHFModem/varaFMModem isn't updated. So, those are always nil, and my code blows up when it tries to dereference them later.

This seems like a bug with the call structure of connect.go, and it being inconsistent... thoughts?

@martinhpedersen
Copy link
Member

I had a quick look, and I think I've spotted the bug 🙂

The problem is not in your code. The problem is initVaraModem(vModem *vara.Modem, scheme string, conf cfg.VaraConfig). It takes a pointer to vara.Modem, expecting to be able to reassign it. It needs to take a pointer to pointer in order to reassign it like that.

So the global vars varaHFModem and varaFMModem is never really assigned and is therefore nil when passed to busyWait(). That's probably a bug from the original author that has been this way since VARA support was first introduced.

I can create a branch to fix this, but I don't have the setup to test. Maybe you could help me with that? What we need to confirm is that the fix does not break teardown on subsequent VARA sessions. E.g. make one connection, wait a bit (without stopping Pat) and then do another connection.

@bseidenberg
Copy link
Author

bseidenberg commented Apr 16, 2023

Yep, happy to test, just shoot me the branch. By the way, if you're on any realtime chat service (IRC?), that'd be great for real-time discussion. Feel free to ping me out of band.

(Edit: Also: Glad we agree on the bug :) )

@martinhpedersen martinhpedersen added this to the v0.14.1 milestone Apr 20, 2023
@martinhpedersen martinhpedersen modified the milestones: v0.14.1, v0.14.2 May 2, 2023
@martinhpedersen martinhpedersen modified the milestones: v0.14.2, v0.15.0 May 19, 2023
@martinhpedersen martinhpedersen modified the milestones: v0.15.0, v0.16.0 May 20, 2023
@martinhpedersen martinhpedersen removed this from the v0.16.0 milestone Apr 13, 2024
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