feat: Add support for custom exception codes #279

CosminPerRam · 2024-09-11T12:22:22Z

Would close #278

This PR is just for a better insight on my problem and suggested changes (although it compiles, tests run successfully and pre-commit is happy).

As in the context of the linked issue, this introduces breaking changes.

uklotzde · 2024-09-11T13:27:38Z

Thanks for contributing!

Some general remarks for discussion.

I have noticed that the naming of the types FunctionCode and Exception is inconsistent. Shouldn't Exception better be renamed to ExceptionCode to reflect this duality?

The different naming of the fallback variants Custom and Other respectively is inconsistent and confusing. That's what I initially stumbled upon. Not sure which term is more common in this context.

CosminPerRam · 2024-09-11T13:38:09Z

The different naming of the fallback variants Custom and Other respectively is inconsistent and confusing.

Shouldn't Exception better be renamed to ExceptionCode to reflect this duality?

Yes, having consistency would be great to see, should this be done in this PR or open another one?

Not sure which term is more common in this context.

I'm not sure either, but personally I'd go with Custom, sounds nicer in this context, as in 'one might have custom implementations' or 'this device returns a custom code'.

uklotzde · 2024-09-11T13:40:56Z

Other in the sense of /// None of the above. would make sense to me.

The documentation of both types should also mention, that encoding one of the predefined variants as Other is possible but not recommended. Unfortunately, this ambiguity cannot be resolved easily.

uklotzde · 2024-09-11T13:42:59Z

We should address the naming of Custom/Other in this PR and defer the renaming of the type(s) to a follow-up PR.

Let's use Custom for now to avoid changing existing code.

src/frame/mod.rs

src/codec/mod.rs

src/frame/mod.rs

src/codec/mod.rs

uklotzde · 2024-09-11T14:17:45Z

src/frame/mod.rs

+    GatewayTargetDevice,
+    /// None of the above.
+    ///
+    /// Although encoding one of the predefined values as this is possible, it is not recommended.


We could then add something like the following hint:

Instead of instantiating this variant directly prefer to use [`Self::new()`] to prevent such ambiguities.

uklotzde · 2024-09-11T14:34:25Z

CHANGELOG.md

@@ -3,6 +3,15 @@

 # Changelog

+## v0.15.0 (Unreleased)
+
+- Added `Exception::Other`.


Suggested change

- Added `Exception::Other`.

- Added `Exception::Custom`.

uklotzde · 2024-09-11T14:35:02Z

src/codec/mod.rs

@@ -385,34 +385,17 @@ impl TryFrom<Bytes> for ExceptionResponse {
            ));
        }
        let function = fn_err_code - 0x80;
-        let exception = Exception::try_from(rdr.read_u8()?)?;
+        let exception = Exception::from(rdr.read_u8()?);


Suggested change

let exception = Exception::from(rdr.read_u8()?);

let exception = Exception::new(rdr.read_u8()?);

uklotzde · 2024-09-11T14:37:32Z

src/codec/mod.rs

-            }
-        };
-        Ok(ex)
+impl From<u8> for Exception {


I suggest to delete this impl From.

As a user of this crate you probably do not want to convert arbitrary u8 values implicitly into a Modbus exception code with .into(). With Exception::new() we now have an explicit constructor to make such a conversion explicit.

I generally try to avoid From/TryFrom implementations between types of different abstraction levels. These should be explicitly converted with properly named methods.

Please then also update the changelog accordingly.

uklotzde · 2024-09-11T15:07:57Z

src/frame/mod.rs

+    GatewayTargetDevice,
+    /// None of the above.
+    ///
+    /// Although encoding one of the predefined values as this is possible, it is not recommended,


Please split into two sentences for readability.

uklotzde

Just one remaining remark.

uklotzde · 2024-09-11T17:55:05Z

Reporting a bug and with a follow-up PR that fixes it is nice. Thank you!

mihairadulescu · 2024-09-11T18:11:49Z

Nice!

CosminPerRam added 3 commits September 11, 2024 15:17

feat: add exception other

e1d0c03

chore: run fmt

75754ce

docs: update CHANGELOG.md

2d14362

uklotzde reviewed Sep 11, 2024

View reviewed changes

src/frame/mod.rs Outdated Show resolved Hide resolved

uklotzde changed the title ~~feat: add exception other~~ feat: Add support for custom exception codes Sep 11, 2024

CosminPerRam added 3 commits September 11, 2024 16:51

docs: exception per-enum doc comment

abdf02b

feat: rename Exception::Other in Exception::Custom

aec1689

docs: add warning on Custom as predefined value

9b6879c

CosminPerRam requested a review from uklotzde September 11, 2024 14:00

fix: Exception::Custom wrong description

703858e

CosminPerRam requested a review from uklotzde September 11, 2024 14:02

uklotzde reviewed Sep 11, 2024

View reviewed changes

src/codec/mod.rs Outdated Show resolved Hide resolved

uklotzde reviewed Sep 11, 2024

View reviewed changes

src/frame/mod.rs Outdated Show resolved Hide resolved

feat: Change Exception From<u8> match name for Custom

640ded1

CosminPerRam requested a review from uklotzde September 11, 2024 14:06

uklotzde reviewed Sep 11, 2024

View reviewed changes

src/codec/mod.rs Outdated Show resolved Hide resolved

uklotzde reviewed Sep 11, 2024

View reviewed changes

CosminPerRam added 3 commits September 11, 2024 17:21

feat: add const new constructor for Exception

40ebaff

docs: alter doc comment of Exception::Custom

eb110a6

feat: alter Exception::new Custom match name

b7519d7

CosminPerRam requested a review from uklotzde September 11, 2024 14:25

uklotzde requested changes Sep 11, 2024

View reviewed changes

CosminPerRam added 2 commits September 11, 2024 17:50

feat: remove From<u8> for Exception completely

ee14c6c

docs: alter CHANGELOG.md

03dd0e9

CosminPerRam requested a review from uklotzde September 11, 2024 14:54

uklotzde reviewed Sep 11, 2024

View reviewed changes

uklotzde approved these changes Sep 11, 2024

View reviewed changes

uklotzde added this to the v0.15.0 milestone Sep 11, 2024

docs: alter Exception::custom comment

7d07457

CosminPerRam requested a review from uklotzde September 11, 2024 17:28

uklotzde merged commit a5eb819 into slowtec:main Sep 11, 2024
10 checks passed

CosminPerRam deleted the feat/exception_other branch September 11, 2024 21:20

CosminPerRam mentioned this pull request Sep 11, 2024

feat: rename Exception to ExceptionCode #281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for custom exception codes #279

feat: Add support for custom exception codes #279

CosminPerRam commented Sep 11, 2024 •

edited

Loading

uklotzde commented Sep 11, 2024 •

edited

Loading

CosminPerRam commented Sep 11, 2024

uklotzde commented Sep 11, 2024

uklotzde commented Sep 11, 2024

uklotzde Sep 11, 2024

uklotzde Sep 11, 2024

uklotzde Sep 11, 2024

uklotzde Sep 11, 2024

uklotzde Sep 11, 2024

uklotzde left a comment

uklotzde commented Sep 11, 2024

mihairadulescu commented Sep 11, 2024

	let exception = Exception::from(rdr.read_u8()?);
	let exception = Exception::new(rdr.read_u8()?);

feat: Add support for custom exception codes #279

feat: Add support for custom exception codes #279

Conversation

CosminPerRam commented Sep 11, 2024 • edited Loading

uklotzde commented Sep 11, 2024 • edited Loading

CosminPerRam commented Sep 11, 2024

uklotzde commented Sep 11, 2024

uklotzde commented Sep 11, 2024

uklotzde Sep 11, 2024

Choose a reason for hiding this comment

uklotzde Sep 11, 2024

Choose a reason for hiding this comment

uklotzde Sep 11, 2024

Choose a reason for hiding this comment

uklotzde Sep 11, 2024

Choose a reason for hiding this comment

uklotzde Sep 11, 2024

Choose a reason for hiding this comment

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Sep 11, 2024

mihairadulescu commented Sep 11, 2024

CosminPerRam commented Sep 11, 2024 •

edited

Loading

uklotzde commented Sep 11, 2024 •

edited

Loading