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

NOISSUE - Migrate gocoap library from v2 to v3.3 #8

Merged
merged 14 commits into from
May 16, 2024

Conversation

felixgateru
Copy link
Contributor

@felixgateru felixgateru commented Apr 19, 2024

What type of PR is this?

This is a feature because it migrates the version of the coap library from v2 to v3.3

What does this do?

  • Migrates the plg-dev\go-coap library from v2 to v3.3
  • Improves CLI by using spf13/cobra

Which issue(s) does this PR fix/relate to?

Resolves https://github.com/absmach/magistrala/issues/2203

Have you included tests for your changes?

Yes, I have included tests for my changes.

Did you document any new/modified feature?

No,

Notes

No,

@felixgateru felixgateru force-pushed the NOISSUE-gocoap-v3 branch 2 times, most recently from bb2d8c1 to 5f9b9fe Compare April 22, 2024 14:16
configs.go Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
@felixgateru felixgateru force-pushed the NOISSUE-gocoap-v3 branch 2 times, most recently from bd529e7 to a8a74bd Compare April 29, 2024 11:25
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
coap/client.go Outdated Show resolved Hide resolved
coap/client.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
Copy link

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

Since we are supporting all functions of coap , we should have test case for all methods with all options with some publicly available coap sever like https://coap.me/crawl/coap://coap.me

README.md Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/coapme/test.sh Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
Copy link

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

Suggestion for KeepClient Connection Alive https://github.com/1998-felix/coap-cli/blob/d4705eed8646741d28d1fd7a2aac0bc9bb987368/coap/client.go#L29-L36

func New(addr string) (Client, error) {
	c, err := udp.Dial(addr, options.WithKeepAlive(10, 60*time.Second, func(cc *client.Conn) { fmt.Println(cc.Ping(cc.Context())) }))
	if err != nil {
		log.Fatalf("Error dialing: %v", err)
	}

	return Client{conn: c}, nil
}

In additonal , It will be great if we provide flags to set have keep Alive time out in client

If flag is not available defaults to 60 seconds.

If KeepAlive Time flag is 0 then it should not add KeepAlive function during connect.

@dborovcanin
Copy link
Contributor

Please resolve conflicts.

@felixgateru felixgateru force-pushed the NOISSUE-gocoap-v3 branch from 9744d39 to 05879cb Compare May 8, 2024 16:53
@felixgateru felixgateru force-pushed the NOISSUE-gocoap-v3 branch from 05879cb to c265729 Compare May 8, 2024 16:56
coap/client.go Outdated Show resolved Hide resolved
coap/client.go Outdated Show resolved Hide resolved
coap/client.go Outdated Show resolved Hide resolved
coap/client.go Show resolved Hide resolved
Signed-off-by: 1998-felix <[email protected]>
README.md Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
coap/client.go Outdated Show resolved Hide resolved
coap/client.go Outdated Show resolved Hide resolved
Copy link

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

Server disconnection idle client leads to new problem
The client doesn't know the disconnection, and client again send disconnection request to server
So we are getting error in server like {"time":"2024-05-16T10:17:46.594334142Z","level":"WARN","msg":"Unsubscribe failed to complete successfully","duration":"31.172803ms","channel_id":"dff59205-90d6-430d-9903-a611cd401bb3","error":"not subscribed"}

@dborovcanin
Copy link
Contributor

Server disconnection idle client leads to new problem The client doesn't know the disconnection, and client again send disconnection request to server So we are getting error in server like {"time":"2024-05-16T10:17:46.594334142Z","level":"WARN","msg":"Unsubscribe failed to complete successfully","duration":"31.172803ms","channel_id":"dff59205-90d6-430d-9903-a611cd401bb3","error":"not subscribed"}

It looks like the problem is that the subscribed client isn't aware of the broken subscription, which makes sense due to the connectionless nature of the underlying UDP. We can merge this one, but @arvindh123 please open a ticket to remind us to double-check this.

@dborovcanin dborovcanin merged commit 826eeae into absmach:master May 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: coap-cli : migrate gocoap library from v2 to v3.3
4 participants