Remove global clients #1014
Remove global clients #1014
Conversation
…OME and HOME directories
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
per1234
added
type: enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1014 +/- ##
==========================================
+ Coverage 20.23% 20.77% +0.53%
==========================================
Files 42 43 +1
Lines 3242 3298 +56
==========================================
+ Hits 656 685 +29
- Misses 2499 2522 +23
- Partials 87 91 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…serial, and update files
…ating logAction method
serial.go
Outdated
| @@ -255,22 +265,22 @@ func (sp *SerialPortList) getPortByName(portname string) *SpPortItem { | |||
| return nil | |||
| } | |||
|
|
|||
| func spErr(err string) { | |||
| func (h *hub) spErr(err string) { | |||
There was a problem hiding this comment.
Can we reorganize those files with one package per struct?
- hub
- serialPortList
- serialHub
It isn't very easy to follow this code
There was a problem hiding this comment.
I thought the same thing, but I think it would be easier for the reviewers if we open a new PR dedicated to refactoring those components. WDYT?
There was a problem hiding this comment.
I tried to move the least as possible to facilitate reviews with the following changes:
- split the
serial.gofile intoserialhub.gowith theserialhubstructserialportlist.gowith theserialPortListand theSpPortItemstruct
- move
spErr,spWriteandspClosefrom theserial.goto thehub.gofile - move the
spHandlerOpenfrom theßerialport.goto thehub.gofile
…iate newSerialHub within it
|
Tested on linux with MKR 1010
|
| @@ -73,51 +110,53 @@ const commands = `{ | |||
| ] | |||
| }` | |||
|
|
|||
| func (h *hub) unregisterConnection(c *connection) { | |||
| if _, contains := h.connections[c]; !contains { | |||
| func (hub *hub) unregisterConnection(c *connection) { | |||
There was a problem hiding this comment.
I think it is more canonical to use single letter for local object mapping
| func (hub *hub) unregisterConnection(c *connection) { | |
| func (h *hub) unregisterConnection(c *connection) { |
| type logWriter struct { | ||
| onWrite func([]byte) | ||
| } |
There was a problem hiding this comment.
I would create an utility that transform a channel to a writer, for instance something like that
// ChanWriter is a simple io.Writer that sends data to a channel.
type ChanWriter struct {
Ch chan<- []byte
}
func (u *ChanWriter) Write(p []byte) (n int, err error) {
u.Ch <- p
return len(p), nil
}then you can directly use it as follows
multiWriter := io.MultiWriter(&ChanWriter{Ch: hub.broadcastSys}, os.Stderr)| } | ||
|
|
||
| func (hub *hub) spErr(err string) { | ||
| //log.Println("Sending err back: ", err) |
There was a problem hiding this comment.
| //log.Println("Sending err back: ", err) |
| // Register serial ports from the connections. | ||
| func (sh *serialhub) Register(port *serport) { | ||
| sh.mu.Lock() | ||
| //log.Print("Registering a port: ", p.portConf.Name) |
There was a problem hiding this comment.
| //log.Print("Registering a port: ", p.portConf.Name) |
| // Unregister requests from connections. | ||
| func (sh *serialhub) Unregister(port *serport) { | ||
| sh.mu.Lock() | ||
| //log.Print("Unregistering a port: ", p.portConf.Name) |
There was a problem hiding this comment.
| //log.Print("Unregistering a port: ", p.portConf.Name) |
| } | ||
|
|
||
| // Register serial ports from the connections. | ||
| func (sh *serialhub) Register(port *serport) { |
There was a problem hiding this comment.
I would use a single char variable
| func (sh *serialhub) Register(port *serport) { | |
| func (s *serialhub) Register(port *serport) { |
| ports map[*serport]bool | ||
| mu sync.Mutex |
There was a problem hiding this comment.
I usually see the other way around first the mutex and then the resource that is protected
| ports map[*serport]bool | |
| mu sync.Mutex | |
| mu sync.Mutex | |
| ports map[*serport]bool |
| for port := range sh.ports { | ||
| if strings.EqualFold(port.portConf.Name, portname) { | ||
| // we found our port | ||
| //spHandlerClose(port) |
| p.OnClose = func(port *serport) { | ||
| hub.serialPortList.MarkPortAsClosed(p.portName) | ||
| hub.serialPortList.List() | ||
| } |
There was a problem hiding this comment.
I think you can remove the OnClose callback and move those two lines of code in the spClose method below after calling p.Close(). AFAIK this struct is the only one closing ports, so moving the code there should cover all the cases. The following should be the updated spClose function:
func (hub *hub) spClose(portname string) {
if myport, ok := hub.serialHub.FindPortByName(portname); ok {
hub.broadcastSys <- []byte("Closing serial port " + portname)
myport.Close()
hub.serialPortList.MarkPortAsClosed(myport.portName)
hub.serialPortList.List()
} else {
hub.spErr("We could not find the serial port " + portname + " that you were trying to close.")
}
}| p := &serport{ | ||
| sendBuffered: make(chan string, 256000), | ||
| sendNoBuf: make(chan []byte), | ||
| sendRaw: make(chan string), | ||
| portConf: conf, | ||
| portIo: sp, | ||
| portName: portname, | ||
| BufferType: buftype, | ||
| } | ||
|
|
||
| p.OnMessage = func(msg []byte) { | ||
| hub.broadcastSys <- msg | ||
| } |
There was a problem hiding this comment.
Instead of this call back I think you could use the ChanWriter I suggested above
| p := &serport{ | |
| sendBuffered: make(chan string, 256000), | |
| sendNoBuf: make(chan []byte), | |
| sendRaw: make(chan string), | |
| portConf: conf, | |
| portIo: sp, | |
| portName: portname, | |
| BufferType: buftype, | |
| } | |
| p.OnMessage = func(msg []byte) { | |
| hub.broadcastSys <- msg | |
| } | |
| p := &serport{ | |
| sendBuffered: make(chan string, 256000), | |
| sendNoBuf: make(chan []byte), | |
| sendRaw: make(chan string), | |
| portConf: conf, | |
| portIo: sp, | |
| portName: portname, | |
| BufferType: buftype, | |
| Writer: &ChanWriter{Ch: hub.broadcastSys}, | |
| } |
Please check if the PR fulfills these requirements
before creating one)
The purpose of this PR is to improve the maintainability, testability, and clarity of the codebase by removing global variables and explicitly passing dependencies as arguments to functions or methods. Global variables can lead to tightly coupled code, making it harder to understand, test, and maintain.
hintohubserial.goand create the following new files:serialhub.gocontains theserialhubstructserialportlist.gocontains theserialPortListand theSpPortItemstructspErr,spWriteandspClosefrom theserial.goto thehub.gofilespHandlerOpenfrom theserialport.goto thehub.gofilego hub.serialPortList.Run()from themaingointo thehub.run()because the hub is in charge of connecting serial events to websocket clients.No.