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

Integrate with github.com/n8jja/Pat-Vara #280

Merged
merged 1 commit into from
Aug 20, 2022

Conversation

xylo04
Copy link
Contributor

@xylo04 xylo04 commented Sep 8, 2021

I would also like to add bandwidth as a transport URL parameter, but that's a bigger change, and not strictly required right now.

@martinhpedersen
Copy link
Member

I would also like to add bandwidth as a transport URL parameter, but that's a bigger change, and not strictly required right now.

I believe this could be implemented in the VARA modem's DialURL function directly? It has access to the full URL, including the query parameters 🙂

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.

This is very exciting 👍

I'm not seing any modem teardown in func cleanup(). The tcp connection to the modem should be closed there, for proper app termination.

And shouldn't the vara package distinguish between modem.Close() and conn.Close()? 🤔 It looks like both the modem connection and the dialed vara connection are represented by the same type (the modem-struct). That's a bit odd IMHO. Wouldn't it be better to let DialURL return a separate type implementing the just the Conn interface?

@xylo04
Copy link
Contributor Author

xylo04 commented Sep 10, 2021

I would also like to add bandwidth as a transport URL parameter, but that's a bigger change, and not strictly required right now.

I believe this could be implemented in the VARA modem's DialURL function directly? It has access to the full URL, including the query parameters 🙂

That's where I'd like to pipe it down, and I already look for the bw query param in the transport code. However, actually putting that into the UI feels like a separate task, especially since other transports might also benefit.

I'm not seing any modem teardown in func cleanup(). The tcp connection to the modem should be closed there, for proper app termination.

Missed that, I'll add it! Also, I wasn't aware it was necessary because the TCP connection was being dropped after VARA disconnect, but it's possible that was a bug on my part. Should the TCP connection stay open after VARA disconnect?

And shouldn't the vara package distinguish between modem.Close() and conn.Close()? 🤔 It looks like both the modem connection and the dialed vara connection are represented by the same type (the modem-struct). That's a bit odd IMHO. Wouldn't it be better to let DialURL return a separate type implementing the just the Conn interface?

I can keep Close for aborting the RF connection and expose a new CloseModem for closing the TCP connection, does that sound right?

But that brings up a tangential point I was confused about. Since golang interfaces are duck-typed, and since net.Listener.Close and net.Conn.Close have the same signature, those two must be the same method, right? I guess that's appropriate in this case?

@xylo04
Copy link
Contributor Author

xylo04 commented Sep 10, 2021

I can keep Close for aborting the RF connection and expose a new CloseModem for closing the TCP connection, does that sound right?

But that brings up a tangential point I was confused about. Since golang interfaces are duck-typed, and since net.Listener.Close and net.Conn.Close have the same signature, those two must be the same method, right? I guess that's appropriate in this case?

The ARDOP and WINMOR stubs both expose Disconnect for RF and Close for Pat's connection to the TNC. Those get used in abortActiveConnection. The PACTOR stub doesn't expose a similar method, but I think it'd be useful to do that for VARA since we're still in early stages. I'll send a PR to add that to http://github.com/n8jja/Pat-Vara.

@xylo04
Copy link
Contributor Author

xylo04 commented Sep 10, 2021

Wouldn't it be better to let DialURL return a separate type implementing the just the Conn interface?

I think I'm starting to grok what you mean here. Maybe NewModem() should only initialize the cmdConn, and then DialURL can initialized dataConn and return that to the client code instead of implementing net.Conn itself. That way, if a client takes the return from DialURL and calls Close on that, they're only closing dataConn, not the modem. I'll start to rework the transport that way.

@xylo04
Copy link
Contributor Author

xylo04 commented Sep 10, 2021

I'm proposing changes to the VARA stub API in n8jja/Pat-Vara#10.

I tried separating Disconnect for just RF and Close for RF and TCP, but it turns out the VARA program doesn't tolerate that well (doesn't seem to like rapid TCP disconnect and reconnect), so I'm sticking with the one modem.Close method.

However, the change does separate out modem.Close from conn.Close as you suggested.

@martinhpedersen
Copy link
Member

I tried separating Disconnect for just RF and Close for RF and TCP, but it turns out the VARA program doesn't tolerate that well (doesn't seem to like rapid TCP disconnect and reconnect), so I'm sticking with the one modem.Close method.

For WINMOR and ARDOP, both data and control sockets are held open even when the modem is idle. Maybe that's what VARA expects as well? Have you looked at the raw tcp frames between VARA and e.g. Winlink Express to see how they've implemented it?

It sounds strange to me that you would need to tear down the modem connection in between sessions. How would that work for incoming p2p connections?

Another concern is the inherit buffering in modems like these. For ARDOP it is very important not to disconnect the data and ctrl sockets until the buffer has been flushed and the modem confirms the disconnect and reaches idle state. If not, the last frames may be lost at either end. Have you investigated this with regards to VARA?

@martinhpedersen
Copy link
Member

@xylo04 - What's the status on this? Are you awaiting my review and merge?

@xylo04
Copy link
Contributor Author

xylo04 commented Sep 25, 2021

@martinhpedersen no, I'm not waiting for review at this point. I'm trying to dig deeper with the TCP connection state as you suggested. To do that, I'm trying to set up both sides of a VARA-FM connection to test more reliably, and that has taken me off on a tangent of setting up my own RMS.

If it helps, we can change this PR to draft for now and I'll mark it active again later.

@martinhpedersen
Copy link
Member

... and that has taken me off on a tangent of setting up my own RMS.

Been there 😆 👍

Thanks for taking the time. My experience is that these transport drivers requires a lot of testing.

I've ordered a RPi 4B in case I have some spare time to do some field testing 🙂 I would also love to see the Listener-interface be implemented. Hopefully I will have time to have a go at it within the next couple of months, if you don't beat me to it 😉

Converting to draft 👍

@xylo04 xylo04 force-pushed the vara branch 5 times, most recently from 35b97a9 to 6719beb Compare March 30, 2022 00:36
@xylo04 xylo04 force-pushed the vara branch 5 times, most recently from 0e2a145 to 4b8e2c9 Compare April 10, 2022 14:09
@xylo04 xylo04 force-pushed the vara branch 5 times, most recently from 3a7e9a2 to 13f7778 Compare April 15, 2022 04:31
@gh42lb
Copy link
Contributor

gh42lb commented Apr 21, 2022 via email

Co-authored-by: gh42lb <92827417+gh42lb@users.noreply.github.com>
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.

I have nothing to add here, except a big thank you for all the hours spent adding support for this mode 😄❤️

Still waiting on my RPi 4 which I ordered many months ago, so I haven't been able to test it myself yet.. but looking forward to it 🙂

@martinhpedersen martinhpedersen added this to the v0.13.0 milestone Aug 20, 2022
@martinhpedersen martinhpedersen merged commit 6756d99 into la5nta:develop Aug 20, 2022
@xylo04 xylo04 deleted the vara branch August 21, 2022 01:25
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.

Is support for VARA in the project timeline?
4 participants