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 varaHFModem/varaFMModem vars not being set on init #398

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

martinhpedersen
Copy link
Member

Ref #387 . As it turns out, the global variables varaHFModem and varaFMModem was never set. It looks like the intention was that func initVaraModem would set these since it sets the pointer value vModem passed as the first argument to the function. However, this is not how it works. In order to do this, the function would need to take a pointer to pointer (vModem **vara.Modem).. but that is just a mess to work with.

To cut this story short, I have fixed this issue. What we now need to do is to confirm that this does not break anything. Since the global vars was never set, the call to vModem.Close() was actually never called and the initialization took place only once.

In order to make sure this still works as intended, I need some help testing the following:

  1. Start pat http
  2. Make a successful VARA connection.
  3. Wait a couple of seconds.
  4. Make another VARA connection.
  5. Gracefully shut down Pat (ctrl+c) and verify that no error is logged.

On step 4, Pat will actually close the current VARA modem connection and re-init the modem as originally intended. This have the potential of failing if Close() does not behave as expected. Step 5 also have the potential of throwing errors, as we now properly Close VARA HF/FM modem connection when Pat shuts down (func cleanup() main.go).

@bseidenberg
Copy link

Step 5 hung at Disconnecting varafm...

Trying to figure out from the stack trace after a SIGQUIT which thread is responsible.

@bseidenberg
Copy link

I can reproduce that hang with just one connect.

@bseidenberg
Copy link

I'm having all sorts of issues with delve hanging... I can't tell for sure, but I think it's somehow getting stuck inside the loop in main.go:276 rather than the vara modem code, but I wouldn't count on that for sure.

@martinhpedersen
Copy link
Member Author

Thanks for debugging with me! We might be able to see what's going on in the logs by setting VARA_DEBUG=true and PAT_DEBUG=true. If your theory is correct, I think we would not see the "Starting cleanup" debug log line. This will also print all commands sent to the VARA modem, which might be helpful.

I don't think I have time to fire up a Windows laptop to get VARA running locally this week, but might be able to look more into it in the following week.

@martinhpedersen
Copy link
Member Author

I think I've found the problem with this. I will push a work-around to this branch, but I would really like to fix this properly by submitting a PR to the vara package repo.

martinhpedersen added a commit to martinhpedersen/Pat-Vara that referenced this pull request Apr 18, 2023
This is needed in order to properly implement a forced/dirty disconnect
in Pat.

Ref la5nta/pat#398
@martinhpedersen
Copy link
Member Author

I ended up amending to the previous commits. I've submitted a PR adding the required method for checking vara's idle state. The current code assumed that if a varaHFModem / varaFMModem is non-nil, the modem is in connected state and thus prevents the shutdown of Pat.

@bseidenberg - Can you help me test this please? 🙏

@bseidenberg
Copy link

bseidenberg commented Apr 19, 2023 via email

@martinhpedersen
Copy link
Member Author

Perfect! Thank you 😀

@martinhpedersen martinhpedersen added this to the v0.14.1 milestone Apr 20, 2023
@martinhpedersen martinhpedersen merged commit c2e3f45 into develop Apr 29, 2023
@martinhpedersen martinhpedersen deleted the vara-init-fix branch April 29, 2023 15:43
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants