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

Multiple modifications for formatting and function #21

Merged
merged 20 commits into from
Jun 28, 2024

Conversation

treavorj
Copy link
Contributor

Here is a partial functional update for the code modifications that I am working on to better use the library for me. So far, all the functional code updates are for the client. You will see some modifications for the server, but they were nonfunctional spelling/grammar updates.
If you do/do not like the direction that this code is going, feel free to let me know and I can try to mesh our visions.
In case I did not make it clear in my issue, great job with the code already! This is just my attempt to add what I view as some polish.

Key updates include:

  • Addition of slog for logging in much of the client for better visibility into function of code
  • Clarity of who owns parameters surrounding the client struct
  • Rework of keepalive to reduce code and merge some functionality
  • Rework of forwardopen to reduce redundant code and clarify usage
  • Spelling updates to reduce listed problems
  • Updates to comments for clarity

Testing:
All automated tests pass.
I have confirmed a large portion (hopefully all) of the functionality that I touched on an AB CompactLogix with no noticed issues.

Treavor and others added 7 commits June 3, 2024 17:16
Updated connect/disconnect functionality
Removed some hard coded parameters
Clarified parameter naming
Spelling fixes
Adjusted examples due to changes in code
Sync localDev with dev
Spelling changes
slog modifications for errors
Use a struct for the list services response.
@treavorj treavorj mentioned this pull request Jun 20, 2024
@danomagnum
Copy link
Owner

Thanks for all the updates. I've started reviewing it and like what I see so far.

It would appear I'm having trouble with the reconnection and noreconnection tests. Might just be a timeout thing, I'll look at it further over the next few days as I get a chance.

Also, I really need to start using a spell checker - I had so many typos.

@treavorj
Copy link
Contributor Author

Haha all good. I get just focusing on the content as that is what is more important at the end of the day.
Let me know if you're still having issues with the connection tests and I can take a look at it early next week (unfortunately off on other things for the exact moment).

client.go Show resolved Hide resolved
connect.go Show resolved Hide resolved
connect.go Outdated Show resolved Hide resolved
connect.go Show resolved Hide resolved
connect.go Show resolved Hide resolved
connect.go Show resolved Hide resolved
getattr.go Outdated Show resolved Hide resolved
connect.go Outdated Show resolved Hide resolved
sendreceive.go Outdated Show resolved Hide resolved
sendreceive.go Outdated Show resolved Hide resolved
@danomagnum
Copy link
Owner

I found the problem with the reconnect test.

See this comment: #21 (comment)

Treavor and others added 12 commits June 25, 2024 09:19
Update test files to current version
Corrected Connect and Disconnect methods to prevent multiple goroutines from triggering the methods
Modify sequence number on Connect to add some randomness to connection ID to allow multiple concurrent clients to one device
… connection limits already

Handle all disconnect in send_recv_data
…e second read that should fail would succeed or fail. (Unknown success due to second read or the keep alive could discover the broken connection)
Update for working tests
@treavorj
Copy link
Contributor Author

@danomagnum Everything should now be up to date and ready for additional review.

@danomagnum danomagnum merged commit e2d292d into danomagnum:master Jun 28, 2024
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.

None yet

2 participants