Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Remove global clients#1014

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
dido18 wants to merge54 commits intomain
base:main
Choose a base branch
Loading
fromremove-global-clients
Open

Remove global clients#1014

dido18 wants to merge54 commits intomainfromremove-global-clients

Conversation

dido18
Copy link
Contributor

@dido18dido18 commentedJan 27, 2025•
edited
Loading

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among thePull Requests
    before creating one)
  • Tests for the changes have been added (for bug fixes / features)
  • What kind of change does this PR introduce?
    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.
  • What is the current behavior?
  • What is the new behavior?

  1. remove global variables, they are now created in the main function and passed explicitly to the components or functions that need them.

    • Tools *tools.Tools
    • Systray systray.Systray
    • Index *index.Resource
    • h = hub
    • loggerWs logWriter
    • serialPorts SerialPortList
    • sh = serialhub

  2. move thego hub.serialPortList.Run() from themaingo into thehub.run() because the hub is in charge of connecting serial events to websocket clients.

  • Does this PR introduce a breaking change?
    No.
  • Other information:

@per1234per1234 added type: enhancementProposed improvement topic: codeRelated to content of the project itself labelsJan 29, 2025
@dido18dido18 changed the base branch frommain todatarace-on-config-accessJanuary 29, 2025 10:34
@dido18dido18 changed the base branch fromdatarace-on-config-access tomainMarch 31, 2025 14:05
@codecov-commenter
Copy link

codecov-commenter commentedMar 31, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is18.87755% with159 lines in your changes missing coverage. Please review.

Project coverage is 20.81%. Comparing base(e7ea376) to head(852f4eb).

Files with missing linesPatch %Lines
hub.go26.08%47 Missing and 4 partials⚠️
serialport.go0.00%28 Missing⚠️
main.go0.00%23 Missing⚠️
serial.go34.28%23 Missing⚠️
update.go0.00%13 Missing⚠️
info.go0.00%10 Missing⚠️
conn.go10.00%9 Missing⚠️
tools/download.go71.42%2 Missing⚠️
Additional details and impacted files
@@            Coverage Diff             @@##             main    #1014      +/-   ##==========================================+ Coverage   20.24%   20.81%   +0.57%==========================================  Files          42       42                Lines        3241     3291      +50     ==========================================+ Hits          656      685      +29- Misses       2498     2515      +17- Partials       87       91       +4
FlagCoverage Δ
unit20.81% <18.87%> (+0.57%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dido18dido18 marked this pull request as ready for reviewApril 1, 2025 13:45
@dido18
Copy link
ContributorAuthor

dido18 commentedApr 3, 2025•
edited
Loading

Tested on linux with MKR 1010

  • upload the echo-skecth and test that the serial monitor works as expected
  • provision an IOT device + update NINA
    • downgrade the NINA with~/.arduino-create/arduino/arduino-fwuploader/2.4.1/arduino-fwuploader firmware flash -b arduino:samd:mkrwifi1010 -a /dev/ttyACM0 --module NINA@1.6.0
    • follow the create a device with iot provisioning

hub.go Outdated
Comment on lines 389 to 392
p.OnClose=func(port*serport) {
hub.serialPortList.MarkPortAsClosed(p.portName)
hub.serialPortList.List()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove theOnClose callback and move those two lines of code in thespClose method below after callingp.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 updatedspClose function:

func (hub*hub)spClose(portnamestring) {ifmyport,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.")}}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

uhm I think it is also called here

@dido18
Copy link
ContributorAuthor

Tested again like#1014 (comment)

@dido18
Copy link
ContributorAuthor

Test: It can download a tool.

This hte was logs

downloadtool avrdude 6.0.1-arduino5{  "DownloadStatus": "Pending",  "Msg": "Ensure that the files are executable"}{  "DownloadStatus": "Pending",  "Msg": "Updating map with location /home/dido/.arduino-create/arduino/avrdude/6.0.1-arduino5"}{  "DownloadStatus": "Success",  "Msg": "Map Updated"}

Copy link
Contributor

@lucarin91lucarin91 left a comment

Choose a reason for hiding this comment

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

I would try to reduce callbacks if possible. They are usually difficult to follow.

hub.go Outdated
Comment on lines 64 to 78
onRegister:=func(port*serport) {
broadcastSys<- []byte("{\"Cmd\":\"Open\",\"Desc\":\"Got register/open on port.\",\"Port\":\""+port.portConf.Name+"\",\"Baud\":"+strconv.Itoa(port.portConf.Baud)+",\"BufferType\":\""+port.BufferType+"\"}")
}
onUnregister:=func(port*serport) {
broadcastSys<- []byte("{\"Cmd\":\"Close\",\"Desc\":\"Got unregister/close on port.\",\"Port\":\""+port.portConf.Name+"\",\"Baud\":"+strconv.Itoa(port.portConf.Baud)+"}")
}
serialHubub:=newSerialHub(onRegister,onUnregister)

onList:=func(data []byte) {
broadcastSys<-data
}
onErr:=func(errstring) {
broadcastSys<- []byte("{\"Error\":\""+err+"\"}")
}
serialPortList:=newSerialPortList(tools,onList,onErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would simplify both of this two with theChanWriter

Suggested change
onRegister:=func(port*serport) {
broadcastSys<- []byte("{\"Cmd\":\"Open\",\"Desc\":\"Got register/open on port.\",\"Port\":\""+port.portConf.Name+"\",\"Baud\":"+strconv.Itoa(port.portConf.Baud)+",\"BufferType\":\""+port.BufferType+"\"}")
}
onUnregister:=func(port*serport) {
broadcastSys<- []byte("{\"Cmd\":\"Close\",\"Desc\":\"Got unregister/close on port.\",\"Port\":\""+port.portConf.Name+"\",\"Baud\":"+strconv.Itoa(port.portConf.Baud)+"}")
}
serialHubub:=newSerialHub(onRegister,onUnregister)
onList:=func(data []byte) {
broadcastSys<-data
}
onErr:=func(errstring) {
broadcastSys<- []byte("{\"Error\":\""+err+"\"}")
}
serialPortList:=newSerialPortList(tools,onList,onErr)
serialHubub:=newSerialHub(ChanWriter{Ch:broadcastSys})
serialPortList:=newSerialPortList(tools,ChanWriter{Ch:broadcastSys})

Comment on lines +56 to +57
OnListfunc([]byte)`json:"-"`
OnErrfunc(string)`json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OnListfunc([]byte)`json:"-"`
OnErrfunc(string)`json:"-"`
writerio.Writer`json:"-"`

serial.go Outdated
sh.mu.Lock()
//log.Print("Registering a port: ", p.portConf.Name)
h.broadcastSys<- []byte("{\"Cmd\":\"Open\",\"Desc\":\"Got register/open on port.\",\"Port\":\""+port.portConf.Name+"\",\"Baud\":"+strconv.Itoa(port.portConf.Baud)+",\"BufferType\":\""+port.BufferType+"\"}")
sh.onRegister(port)
Copy link
Contributor

@lucarin91lucarin91Apr 18, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
sh.onRegister(port)
fmt.Fprintf(sh.writer,`{"Cmd":"Open","Desc":"Got register/open onport.","Port":%q,"Baud":%d,"BufferType":%q}`,port.portConf.Name ,port.portConf.Baud,port.BufferType)

sh.mu.Lock()
//log.Print("Unregistering a port: ", p.portConf.Name)
h.broadcastSys<- []byte("{\"Cmd\":\"Close\",\"Desc\":\"Got unregister/close on port.\",\"Port\":\""+port.portConf.Name+"\",\"Baud\":"+strconv.Itoa(port.portConf.Baud)+"}")
sh.onUnregister(port)
Copy link
Contributor

@lucarin91lucarin91Apr 18, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
sh.onUnregister(port)
fmt.Fprintf(sh.writer,`{"Cmd":"Close","Desc":"Got unregister/close onport.","Port":%q,"Baud":%d}`,port.portConf.Name,port.portConf.Baud)

Comment on lines +108 to +109
OnList:onList,
OnErr:onErr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OnList:onList,
OnErr:onErr,
writer:io.Writer,

//log.Println(err)
h.broadcastSys<- []byte("Error creating json on port list "+
err.Error())
sp.OnErr("Error creating json on port list "+err.Error())
Copy link
Contributor

@lucarin91lucarin91Apr 18, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
sp.OnErr("Error creating json on port list"+err.Error())
fmt.Fprintf(sp.writer,"Error creating json on port list%s",err.Error())

sp.OnErr("Error creating json on port list "+err.Error())
}else {
h.broadcastSys<-ls
sp.OnList(ls)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sp.OnList(ls)
sp.Write(ls)

@lucarin91lucarin91 dismissed theirstale reviewApril 18, 2025 14:42

I added some comments but I don't want to block the PR

@cmaglie
Copy link
Member

cmaglie commentedApr 24, 2025•
edited
Loading

I've merged#1032 here.

@alessio-peruginialessio-perugini removed their request for reviewAugust 6, 2025 19:52
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@lucarin91lucarin91lucarin91 left review comments

@cmagliecmagliecmaglie approved these changes

+1 more reviewer

@alessio-peruginialessio-peruginialessio-perugini approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
topic: codeRelated to content of the project itselftype: enhancementProposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@dido18@codecov-commenter@cmaglie@lucarin91@alessio-perugini@per1234
[8]ページ先頭
©2009-2025 Movatter.jp