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

Add simple server #89

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Add simple server #89

merged 1 commit into from
Apr 10, 2024

Conversation

guenhter
Copy link
Contributor

@guenhter guenhter commented Apr 5, 2024

The functionality for starting a simple server is added. It is not possible yet with to also add nodes. Only a empty server is started which can be connected to.

Copy link
Contributor

@sgoll sgoll left a comment

Choose a reason for hiding this comment

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

Wow, thank you! 🚀

Can you fix the CI errors? They mainly deal with missing docs and some minor nitpicking from clippy. You should be able to reproduce most of these with cargo clippy.

src/ua/server.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
@guenhter guenhter force-pushed the add-simple-server branch 2 times, most recently from 2b236ec to a8f19d6 Compare April 8, 2024 06:51
@sgoll
Copy link
Contributor

sgoll commented Apr 8, 2024

Can you fix the remaining errors before we merge this?

@guenhter
Copy link
Contributor Author

guenhter commented Apr 8, 2024

:) feels like more errors than added lines of code. Yes, I'll look into it. Thx

@uklotzde
Copy link
Collaborator

uklotzde commented Apr 8, 2024

:) feels like more errors than added lines of code. Yes, I'll look into it. Thx

We have agreed on applying aggressive linting rules regarding both code and documentation. Rust gives us the tools and we are using them. Should hopefully pay off in the long term 😅

@guenhter
Copy link
Contributor Author

guenhter commented Apr 9, 2024

Totally agree. The first time is always some hard but after getting used to it, I guess it is no big deal any more.

@guenhter guenhter force-pushed the add-simple-server branch 2 times, most recently from 812283d to b68c649 Compare April 9, 2024 08:48
@uklotzde
Copy link
Collaborator

uklotzde commented Apr 9, 2024

Totally agree. The first time is always some hard but after getting used to it, I guess it is no big deal any more.

Installing and running pre-commit locally will reduce the feedback times.

Our .justfile contains the required commands in the setup and pre-commit recipes. Not yet mentioned in the README 🙈

@guenhter guenhter changed the title Add simple server DRAFT Add simple server Apr 10, 2024
@guenhter
Copy link
Contributor Author

I've pushed a few more things to this to make the simple server be a little bit more useful.

Note, that I'm not the advanced Rust dev yet, so chances are high, that I do things wrong here from a Rust point of view or that I did something which is not how you intended to be done.

@guenhter
Copy link
Contributor Author

What is the idea behind the package Client and ua::Client? I did the same with the server but at this point, I'm keen to remove the ua::Server and pack everything just into the Server. Any remarks on that?

@sgoll
Copy link
Contributor

sgoll commented Apr 10, 2024

What is the idea behind the package Client and ua::Client? I did the same with the server but at this point, I'm keen to remove the ua::Server and pack everything just into the Server. Any remarks on that?

The idea is that the types in the ua module are thin wrappers around the underlying structures from open62541-sys, with names and methods mostly intact and following the same principles as the underlying library. They should add only what is necessary to make their interface safe (or explicitly marked unsafe, but that should be restricted to crate-local methods), but no additional business logic.

On the other hand, the non-ua client API is allowed to deviate from the open62541 naming and methods, adding a more Rust-idiomatic interface (which is still pretty much in flow at this point and far from being set in stone).


That being said, the distinction is not as clear as it should be: some of the methods or rather free helper functions from async_client.rs could be moved to ua/client.rs, e.g. service_request()ua::Client::async_service().

The functionality for starting a simple server is added. It
is not possible yet with to also add nodes. Only a empty server
is started which can be connected to.
Copy link
Contributor

@sgoll sgoll left a comment

Choose a reason for hiding this comment

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

Thank you for all the improvements! 🎉

@sgoll sgoll merged commit d809919 into HMIProject:main Apr 10, 2024
13 checks passed
@guenhter guenhter changed the title DRAFT Add simple server Add simple server Apr 10, 2024
@guenhter guenhter deleted the add-simple-server branch April 10, 2024 13:31
sgoll added a commit that referenced this pull request Apr 10, 2024
## Description

This cleans up some of the data structures introduced in #89, fixing
issues where ownership passed into the C library where it was not
expected, potentially leaking memory. See the individual commits in the
PR for details.
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.

3 participants