harenber / ptc-go

A driver for SCS PACTOR modems for Pat
MIT License
8 stars 2 forks source link

Make API cleaner and more idomatic #25

Open blockmurder opened 4 years ago

blockmurder commented 4 years ago

Yes, the API is really starting to take shape now :)

I have another suggestion: Maybe you could also add a Conn struct to represent a dialed connection? If you move the net.Conn methods (Read, Write, SetDeadline..., Close...) to that struct, I believe the API would be even cleaner and more idiomatic.

type Modem
    func OpenModem(path string, baudRate int, myCall string, initScript string) (p *Modem, err error)
    func (p *Modem) DialURL(url *transport.URL) (net.Conn, error)
    func (p *Modem) Listen() (ln net.Listener, err error)
    func (p *Modem) Close() error // Closes the serial connection to the modem

type Conn
    func (c *Conn) Read(d []byte) (int, error)
    func (c *Conn) Write(d []byte) (int, error)
    func (c *Conn) Close() error // Disconnect (should also flush before disconnect)
    func (c *Conn) SetDeadline(t time.Time) error
    func (c *Conn) SetReadDeadline(t time.Time) error
    func (c *Conn) SetWriteDeadline(t time.Time) error
    func (c *Conn) LocalAddr() net.Addr
    func (c *Conn) RemoteAddr() net.Addr
    func (c *Conn) Flush() (err error)
    func (c *Conn) TxBufferLen() int

What do you think?

Originally posted by @martinhpedersen in https://github.com/harenber/ptc-go/issues/7#issuecomment-534721940

martinhpedersen commented 4 years ago

Simplest solution would probably be to wrap *Modem in a Conn struct and move all the relevant methods to the Conn struct like this:

func (m *Modem) DialURL(...) (net.Conn, error) {
  // Here you must dial the connection before returning of course
  return &Conn{m}
}

type Conn struct { m *Modem }

func (c *Conn) Read(d []byte) (int, error) {
  // Code from Modem.Read()
}

func (c *Conn) Write(d []byte) (int, error) {
  // Code from Modem.Write()
}

func (c *Conn) Close() error {
  // Here you must ensure that only the connection is closed, not the underlying serial connection to the modem.
}
...

Get it?

blockmurder commented 4 years ago

@martinhpedersen could you have a look at commit 9ab18eb80da0e595ef80675ee40875ab1b3d0512 The driver has now two different handles (Modem) and (Conn). Unfortunately, after connection could not be established (Link setup failed), the pModem reference does not get deleted. Trying a second time causes PAT to panic:

2019/10/09 09:52:47 Unable to get frequency from rig mauna_loa: read tcp 127.0.0.1:39560->127.0.0.1:4532: i/o timeout.
2019/10/09 09:52:47 Starting HTTP service (:8080)...

WARNING: You have enable GPSd HTTP endpoint (enable_http). You might expose
         your current position to anyone who has access to the Pat web interface!

2019/10/09 09:52:58 Initialising pactor modem
2019/10/09 09:52:59 EOF
2019/10/09 09:52:59 write: 
2019/10/09 09:52:59 write: 
2019/10/09 09:52:59 write: Quit
2019/10/09 09:52:59 Running init commands
2019/10/09 09:52:59 write: MYcall XYZ
2019/10/09 09:52:59 write: PTCH 31
2019/10/09 09:52:59 write: MAXE 35
2019/10/09 09:52:59 write: CM 0
2019/10/09 09:52:59 write: REM 0
2019/10/09 09:53:00 write: BOX 0
2019/10/09 09:53:00 write: MAIL 0
2019/10/09 09:53:00 write: CHOB 0
2019/10/09 09:53:00 write: UML 1
2019/10/09 09:53:00 write: TONES 4
2019/10/09 09:53:00 write: MARK 1600
2019/10/09 09:53:00 write: SPACE 1400
2019/10/09 09:53:00 write: CWID 0
2019/10/09 09:53:00 write: CONType 3
2019/10/09 09:53:00 write: MODE 0
2019/10/09 09:53:01 start hostmode
2019/10/09 09:53:01 write: JHOST1
2019/10/09 09:53:02 EOF
2019/10/09 09:53:02 start status thread
2019/10/09 09:53:02 start receive thread
2019/10/09 09:53:02 start send thread
2019/10/09 09:53:02 QSY pactor: 7050.000
2019/10/09 09:53:02 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:02 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:03 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:04 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:04 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:05 Connecting to ZYX (pactor)...
2019/10/09 09:53:05 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:05 {a:0 b:0 c:0 d:0 e:0 f:0}
2019/10/09 09:53:06 Write (7): C ZYX
2019/10/09 09:53:06 {a:0 b:0 c:1 d:0 e:1 f:1}
2019/10/09 09:53:07 {a:0 b:0 c:1 d:0 e:1 f:1}
2019/10/09 09:53:07 {a:0 b:0 c:1 d:0 e:2 f:1}
2019/10/09 09:53:08 {a:0 b:0 c:1 d:0 e:2 f:1}
2019/10/09 09:53:08 {a:0 b:0 c:1 d:0 e:2 f:1}
2019/10/09 09:53:09 {a:0 b:0 c:1 d:0 e:3 f:1}
2019/10/09 09:53:10 {a:0 b:0 c:1 d:0 e:3 f:1}
2019/10/09 09:53:10 {a:0 b:0 c:1 d:0 e:4 f:1}
2019/10/09 09:53:11 {a:0 b:0 c:1 d:0 e:4 f:1}
2019/10/09 09:53:11 {a:0 b:0 c:1 d:0 e:5 f:1}
2019/10/09 09:53:12 {a:0 b:0 c:1 d:0 e:5 f:1}
2019/10/09 09:53:13 {a:0 b:0 c:1 d:0 e:6 f:1}
2019/10/09 09:53:13 {a:0 b:0 c:1 d:0 e:6 f:1}
2019/10/09 09:53:14 {a:0 b:0 c:1 d:0 e:7 f:1}
2019/10/09 09:53:14 {a:0 b:0 c:1 d:0 e:7 f:1}
2019/10/09 09:53:15 {a:0 b:0 c:1 d:0 e:8 f:1}
2019/10/09 09:53:16 {a:0 b:0 c:1 d:0 e:8 f:1}
2019/10/09 09:53:16 {a:0 b:0 c:1 d:0 e:9 f:1}
2019/10/09 09:53:17 {a:0 b:0 c:1 d:0 e:9 f:1}
2019/10/09 09:53:17 {a:0 b:0 c:1 d:0 e:10 f:1}
2019/10/09 09:53:18 {a:0 b:0 c:1 d:0 e:10 f:1}
2019/10/09 09:53:19 {a:0 b:0 c:1 d:0 e:11 f:1}
2019/10/09 09:53:19 {a:0 b:0 c:1 d:0 e:11 f:1}
2019/10/09 09:53:20 {a:0 b:0 c:1 d:0 e:12 f:1}
2019/10/09 09:53:20 {a:0 b:0 c:1 d:0 e:12 f:1}
2019/10/09 09:53:21 {a:0 b:0 c:1 d:0 e:13 f:1}
2019/10/09 09:53:22 {a:0 b:0 c:1 d:0 e:13 f:1}
2019/10/09 09:53:22 {a:0 b:0 c:1 d:0 e:14 f:1}
2019/10/09 09:53:23 {a:0 b:0 c:1 d:0 e:14 f:1}
2019/10/09 09:53:23 {a:0 b:0 c:1 d:0 e:15 f:1}
2019/10/09 09:53:24 {a:0 b:0 c:1 d:0 e:15 f:1}
2019/10/09 09:53:25 {a:0 b:0 c:1 d:0 e:15 f:1}
2019/10/09 09:53:25 {a:0 b:0 c:1 d:0 e:16 f:1}
2019/10/09 09:53:26 {a:0 b:0 c:1 d:0 e:16 f:1}
2019/10/09 09:53:26 {a:0 b:0 c:1 d:0 e:17 f:1}
2019/10/09 09:53:27 {a:0 b:0 c:1 d:0 e:17 f:1}
2019/10/09 09:53:28 {a:0 b:0 c:1 d:0 e:18 f:1}
2019/10/09 09:53:28 {a:0 b:0 c:1 d:0 e:18 f:1}
2019/10/09 09:53:29 {a:0 b:0 c:1 d:0 e:19 f:1}
2019/10/09 09:53:29 {a:0 b:0 c:1 d:0 e:19 f:1}
2019/10/09 09:53:30 {a:0 b:0 c:1 d:0 e:20 f:1}
2019/10/09 09:53:31 {a:0 b:0 c:1 d:0 e:20 f:1}
2019/10/09 09:53:31 {a:0 b:0 c:1 d:0 e:21 f:1}
2019/10/09 09:53:32 {a:0 b:0 c:1 d:0 e:21 f:1}
2019/10/09 09:53:32 {a:0 b:0 c:1 d:0 e:22 f:1}
2019/10/09 09:53:33 {a:0 b:0 c:1 d:0 e:22 f:1}
2019/10/09 09:53:34 {a:0 b:0 c:1 d:0 e:23 f:1}
2019/10/09 09:53:34 {a:0 b:0 c:1 d:0 e:23 f:1}
2019/10/09 09:53:35 {a:0 b:0 c:1 d:0 e:24 f:1}
2019/10/09 09:53:35 {a:0 b:0 c:1 d:0 e:24 f:1}
2019/10/09 09:53:36 {a:0 b:0 c:1 d:0 e:25 f:1}
2019/10/09 09:53:37 {a:0 b:0 c:1 d:0 e:25 f:1}
2019/10/09 09:53:37 {a:0 b:0 c:1 d:0 e:26 f:1}
2019/10/09 09:53:38 {a:0 b:0 c:1 d:0 e:26 f:1}
2019/10/09 09:53:38 {a:0 b:0 c:1 d:0 e:27 f:1}
2019/10/09 09:53:39 {a:0 b:0 c:1 d:0 e:27 f:1}
2019/10/09 09:53:40 {a:0 b:0 c:1 d:0 e:28 f:1}
2019/10/09 09:53:40 {a:0 b:0 c:1 d:0 e:28 f:1}
2019/10/09 09:53:41 {a:0 b:0 c:1 d:0 e:28 f:1}
2019/10/09 09:53:41 {a:0 b:0 c:1 d:0 e:29 f:1}
2019/10/09 09:53:42 {a:0 b:0 c:1 d:0 e:29 f:1}
2019/10/09 09:53:43 {a:0 b:0 c:1 d:0 e:30 f:1}
2019/10/09 09:53:43 {a:0 b:0 c:1 d:0 e:30 f:1}
2019/10/09 09:53:44 {a:0 b:0 c:1 d:0 e:31 f:1}
2019/10/09 09:53:44 {a:0 b:0 c:1 d:0 e:31 f:1}
2019/10/09 09:53:45 {a:0 b:0 c:1 d:0 e:32 f:1}
2019/10/09 09:53:46 {a:0 b:0 c:1 d:0 e:32 f:1}
2019/10/09 09:53:46 {a:0 b:0 c:1 d:0 e:33 f:1}
2019/10/09 09:53:47 {a:0 b:0 c:1 d:0 e:33 f:1}
2019/10/09 09:53:48 {a:0 b:0 c:1 d:0 e:34 f:1}
2019/10/09 09:53:48 {a:0 b:0 c:1 d:0 e:34 f:1}
2019/10/09 09:53:49 {a:0 b:0 c:1 d:0 e:35 f:1}
2019/10/09 09:53:49 {a:0 b:0 c:1 d:0 e:35 f:1}
2019/10/09 09:53:50 {a:1 b:1 c:0 d:0 e:0 f:0}
2019/10/09 09:53:50 Link setup failed
2019/10/09 09:53:50 waiting for all threads to exit
2019/10/09 09:53:50 exit status thread
2019/10/09 09:53:51 exit receive thread
2019/10/09 09:53:51 exit send thread
2019/10/09 09:53:51 write: 
2019/10/09 09:53:51 Unable to establish connection to remote: Link setup failed
2019/10/09 09:53:52 QSX pactor: 14213.000
2019/10/09 09:54:37 Close called from /go/src/github.com/la5nta/pat/connect.go#309
2019/10/09 09:54:37 http: panic serving 172.17.0.2:57556: send on closed channel
goroutine 10 [running]:
net/http.(*conn).serve.func1(0xc0003f0000)
    /usr/local/go/src/net/http/server.go:1767 +0x139
panic(0x9334a0, 0xa8d740)
    /usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/la5nta/pat/vendor/github.com/harenber/ptc-go/ptc.(*Modem).send(...)
    /go/src/github.com/la5nta/pat/vendor/github.com/harenber/ptc-go/ptc/modem.go:456
github.com/la5nta/pat/vendor/github.com/harenber/ptc-go/ptc.(*Modem).forceDisconnect(...)
    /go/src/github.com/la5nta/pat/vendor/github.com/harenber/ptc-go/ptc/modem.go:451
github.com/la5nta/pat/vendor/github.com/harenber/ptc-go/ptc.(*Modem).Close(0xc0002f00f0, 0x0, 0x0)
    /go/src/github.com/la5nta/pat/vendor/github.com/harenber/ptc-go/ptc/modem.go:206 +0x50c
main.initPactorModem(0xc0002d0315, 0x1d)
    /go/src/github.com/la5nta/pat/connect.go:309 +0x181
main.Connect(0xc0002d0315, 0x1d, 0x0)
    /go/src/github.com/la5nta/pat/connect.go:67 +0x12d8
main.ConnectHandler(0xaa1aa0, 0xc0000201c0, 0xc0002ec500)
    /go/src/github.com/la5nta/pat/http.go:391 +0x9b
net/http.HandlerFunc.ServeHTTP(0xa065c8, 0xaa1aa0, 0xc0000201c0, 0xc0002ec500)
    /usr/local/go/src/net/http/server.go:2007 +0x44
github.com/la5nta/pat/vendor/github.com/gorilla/mux.(*Router).ServeHTTP(0xc000194690, 0xaa1aa0, 0xc0000201c0, 0xc0002ec500)
    /go/src/github.com/la5nta/pat/vendor/github.com/gorilla/mux/mux.go:159 +0x104
net/http.(*ServeMux).ServeHTTP(0xe7dce0, 0xaa1aa0, 0xc0000201c0, 0xc0002ec300)
    /usr/local/go/src/net/http/server.go:2387 +0x1bd
net/http.serverHandler.ServeHTTP(0xc00009e0e0, 0xaa1aa0, 0xc0000201c0, 0xc0002ec300)
    /usr/local/go/src/net/http/server.go:2802 +0xa4
net/http.(*conn).serve(0xc0003f0000, 0xaa2e20, 0xc0002be240)
    /usr/local/go/src/net/http/server.go:1890 +0x875
created by net/http.(*Server).Serve
    /usr/local/go/src/net/http/server.go:2927 +0x38e
q^C
martinhpedersen commented 4 years ago

I don't think the Modem should be de-initialized when the conn.Close() method is called. Only the associated connection should be disconnected and closed (or noop if it's already closed) so that the Modem instance can be used for subsequent DialURL calls (and future Listen/Accept).

Also, calling modem.Close() on an already closed Modem should be a noop. You could guard against that by checking m.flags.closed in modem.Close().

I believe fixing those two details will resolve your problem :)

harenber commented 2 years ago

Think the problem in https://github.com/harenber/ptc-go/issues/25#issuecomment-539937441 will also be fixed by the listen mode, which I follow in #7