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

Implement Report Server ID (function code 17) #274

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

snejugal
Copy link
Contributor

@snejugal snejugal commented Sep 9, 2024

This MR implements the Report Server ID function (code 17) from the spec.

I tried to work around the absence of this function using Custom requests and responses, but it turns out tokio-modbus does not support them properly. Since this function is defined in the spec, I decided to add native support for this function.

@uklotzde
Copy link
Member

uklotzde commented Sep 9, 2024

Welcome and thank you for contributing!

If you have ideas on how to improve the situation around Custom function calls please also consider to submit a PR.

@uklotzde
Copy link
Member

uklotzde commented Sep 9, 2024

Please ignore the pre-commit failure. I'll take care of that separately.

After merging #275 this should be fixed.

src/frame/mod.rs Outdated Show resolved Hide resolved
src/codec/mod.rs Show resolved Hide resolved
@uklotzde
Copy link
Member

uklotzde commented Sep 9, 2024

Merging main will fix the pre-commit failure.

@snejugal
Copy link
Contributor Author

Rebased on main, the precommit check should pass now.

As of support for custom functions, I think that a trait-based solution might be useful, though that will be a massive overhaul of the crate. Nonetheless, I will try to describe this idea in #123 so it can get some feedback before being implemented (by me or by someone else).

@uklotzde
Copy link
Member

In most cases you don't need to rebase. This would invalidate previous review comment. Each PR will be squashed into a single commit when merged.

src/frame/mod.rs Outdated Show resolved Hide resolved
@uklotzde
Copy link
Member

As of support for custom functions, I think that a trait-based solution might be useful, though that will be a massive overhaul of the crate.

Both the public API and the internal implementation could be improved and optimized in many ways.

@uklotzde uklotzde merged commit abb8539 into slowtec:main Sep 10, 2024
10 checks passed
@uklotzde
Copy link
Member

Technically this is a breaking change because FunctionCode is not marked as non_exhaustive (which should probably be added). But I tend to release it as v0.14.1.

Opinions?

@uklotzde
Copy link
Member

The breaking change only affects the (experimental) server-side API. Another reason why this should not be bundled in a single crate together with the client-side API.

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.

2 participants