Remove Magic Numbers for Enum IDs and replace with constants by parth-20-07 · Pull Request #37 · luminartech/uds_protocol

parth-20-07 · 2025-10-17T19:24:57Z

Implementation:

num_enum procedural macros to use the determinant of unit enum to implement into, try_from methods.
Implement constants for non-unit type enums to replace the magic numbers with constants in the into/try_from methods.

Closes: https://github.com/luminartech/dft/issues/328

Signed-off-by: Parth Patel parth.pmech@gmail.com

- uses procedural macros to use the determinant of enum to convert number<>enum Signed-off-by: Parth Patel <parth.pmech@gmail.com>

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

codecov · 2025-10-17T19:26:18Z

Codecov Report

❌ Patch coverage is 33.59684% with 336 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/service.rs	0.00%	113 Missing ⚠️
src/common/negative_response_code.rs	0.00%	80 Missing ⚠️
src/services/read_dtc_information.rs	36.89%	65 Missing ⚠️
src/common/diagnostic_identifier.rs	31.57%	52 Missing ⚠️
src/common/dtc_status.rs	31.57%	13 Missing ⚠️
src/common/dtc_ext_data.rs	37.50%	10 Missing ⚠️
src/common/dtc_snapshot.rs	0.00%	3 Missing ⚠️

Files with missing lines	Coverage Δ
src/common/communication_control_type.rs	`97.05% <100.00%> (+0.13%)`	⬆️
src/common/communication_type.rs	`94.73% <ø> (-2.33%)`	⬇️
src/common/diagnostic_session_type.rs	`94.91% <100.00%> (+0.17%)`	⬆️
src/common/reset_type.rs	`96.77% <100.00%> (+0.10%)`	⬆️
src/common/security_access_type.rs	`96.72% <100.00%> (+0.11%)`	⬆️
src/lib.rs	`100.00% <100.00%> (+22.22%)`	⬆️
src/services/control_dtc_settings.rs	`100.00% <100.00%> (ø)`
src/services/request_file_transfer.rs	`97.19% <100.00%> (+<0.01%)`	⬆️
src/services/routine_control.rs	`93.39% <100.00%> (+0.12%)`	⬆️
src/common/dtc_snapshot.rs	`85.71% <0.00%> (ø)`
... and 6 more

Copilot

Pull Request Overview

This PR refactors the codebase to eliminate magic numbers by introducing named constants for enum discriminants and implementing automatic conversion traits using the num_enum crate.

Key changes:

Introduces num_enum procedural macros for unit enums to auto-derive Into and TryFrom implementations
Adds named constants for non-unit enum types to replace magic numbers in conversion methods
Updates error handling to use try_from instead of infallible from conversions where appropriate

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Cargo.toml	Adds `num_enum` dependency (version 0.7.4) for enum conversion macros
src/lib.rs	Applies `num_enum` derives to `RoutineControlSubFunction` and `DtcSettings`, adds test cases, removes manual `From`/`TryFrom` implementations
src/error.rs	Adds new error variant `InvalidUDSMessageValue` for wire format validation
src/service.rs	Adds constants for service type IDs and response offsets, refactors methods to use constants
src/services/routine_control.rs	Changes from infallible `from` to `try_from` with error handling
src/services/control_dtc_settings.rs	Changes from infallible `from` to `try_from` with error handling
src/services/request_file_transfer.rs	Adds constants for file operation modes, updates match statements
src/services/read_dtc_information.rs	Adds extensive constants for DTC subfunction IDs, refactors decode/encode methods, introduces `response_id()` helper
src/common/*	Multiple files updated with constants replacing magic numbers in enums and conversion methods
src/common/communication_type.rs	Applies `num_enum` derives, removes manual trait implementations

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-20T16:48:04Z

+                    UserDefMemoryDTCSnapshotRecordByDTCNumRecord::decode(reader)?.unwrap(),
+                )))
+            }
+            Self::USER_DEF_MEMORY_DTC_EXT_DATA_RECORD_BY_DTC_NUMBER => todo!(),


The decode implementation contains a todo!() macro that will panic at runtime if this case is encountered. This represents incomplete functionality that should be implemented before merging.

Suggested change

Self::USER_DEF_MEMORY_DTC_EXT_DATA_RECORD_BY_DTC_NUMBER => todo!(),

Self::USER_DEF_MEMORY_DTC_EXT_DATA_RECORD_BY_DTC_NUMBER => {

Ok(Some(Self::UserDefMemoryDTCExtDataRecordByDTCNumberList(

UserDefMemoryDTCExtDataRecordByDTCNumRecord::decode(reader)?.unwrap(),

)))

}

Incorrect Suggestion. DTC Is not implemented for decode.

Copilot

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

kymikoloco

The negative response codes and DIDs could really benefit from an upgrade of the num_enum macro, I really don't know how to feel about not just using the discriminant and replacing everything with screaming caps.

I especially don't think that replacing the ranges with 2 constant variables is the right way to go about it.

kymikoloco · 2025-10-20T19:59:28Z

-    VehicleManufacturerECUSoftwareNumber = 0xF188,
-    VehicleManufacturerECUSoftwareVersionNumber = 0xF189,
-    SystemSupplierIdentifier = 0xF18A,
+    BootSoftwareIdentification = Self::BOOT_SOFTWARE_IDENTIFICATION,


This is one of those instances that I would like to be set as the raw value in the struct definition just for the easy readability of no screaming caps. I kind of wish num_enum supported variant types, because they support a single enum as multiple numbers via the alternatives, but not storing them?

I don't know, I don't think these count as magic numbers, and I would like to use a different scheme for the DIDs and RIDs (like our own macro, or Zach's suggestion that we just add the derive macros to the uds_protocol_derive crate, which may allow us to implement this missing feature.

Can we just revert the changes to the DIDs for now? I am not a fan of the change specifically to these.

kymikoloco · 2025-10-20T20:01:19Z

+            UDSIdentifier::ISO_SAE_RESERVED_START..=UDSIdentifier::ISO_SAE_RESERVED_END => {
+                UDSIdentifier::ISOSAEReserved(value)
+            }
+            UDSIdentifier::VEHICLE_MANUFACTURER_SPECIFIC_START
+                ..=UDSIdentifier::VEHICLE_MANUFACTURER_SPECIFIC_END => {
+                UDSIdentifier::VehicleManufacturerSpecific(value)
+            }


The constants in the range match alone looks really gnarly from a readability standpoint and I don't think we should be doing this 0_0

kymikoloco · 2025-10-21T15:14:14Z

    ///
    /// Will be 0 after a successful [`ClearDiagnosticInformation`](crate::services::ClearDiagnosticInformation) service
-    TestFailed,
+    TestFailed = Self::TEST_FAILED,


This doesn't have any magic numbers due to the fact the bitmask attribute handles all the bitwise operations. I don't think we need constants in this enum, it could cause someone to cast to u8 to do checks instead of using the newtype to do all bitwise comparisons

kymikoloco · 2025-10-21T15:26:24Z

+    fn test_incorrect_discriminant() {
+        let raw_discriminant: u8 = 0x04;
+
+        let id_from_discriminant = RoutineControlSubFunction::try_from(raw_discriminant)
+            .err()
+            .unwrap()
+            .to_string();
+
+        assert_eq!(
+            id_from_discriminant,
+            Error::InvalidUDSMessageValue(raw_discriminant).to_string()
+        );


The discriminant is a programming term, not something we need to test or even call out. You're testing an incorrect value.

kymikoloco · 2025-10-21T15:29:00Z

+#[derive(Clone, Copy, Debug, Eq, IntoPrimitive, PartialEq, TryFromPrimitive)]
+#[num_enum(error_type(name = crate::Error, constructor = Error::InvalidUDSMessageValue))]
+#[repr(u8)]
 pub enum DtcSettings {
-    On,
-    Off,
+    On = 0x01,
+    Off = 0x02,
 }


Looking at the UDS docs, every other value is represented, we just didn't implement the rest.

kymikoloco · 2025-10-21T15:35:51Z

+#[repr(u8)]
 pub enum UdsServiceType {


Must do: Why was this not a TryFromPrimitive? We could probably greatly simplify this and use the num_enum alternatives configuration since each of these can contain 2 values, the request and the response.

Then the functions here can use try_from and add the constant offset in a single call.

kymikoloco · 2025-10-21T16:54:17Z

+            Self::ISO_RESERVED_START..=Self::ISO_RESERVED_END => Ok(Self::ISOSAEReserved(value)),
+            Self::VEHICLE_MANUFACTURER_START..=Self::VEHICLE_MANUFACTURER_END => {
+                Ok(Self::VehicleManufacturerSpecific(value))
+            }
+            Self::SYSTEM_SUPPLIER_START..=Self::SYSTEM_SUPPLIER_END => {
+                Ok(Self::SystemSupplierSpecific(value))
+            }


Might be able to do stuff like this:

fn is_iso_reserved(val: u8) -> bool { (0x00 == val) || (0x06..=0x3F).contains(&val) || (0x7F == val) } match value { v if is_iso_reserved(v) => Self::ISOSAEReserved(v), other_vals => pseudocode, }

parth-20-07 · 2025-11-04T21:45:19Z

@kymikoloco @zheylmun Closing this PR due to closed discussions on the constants requirements. Can be reopened in future if new discussions are initiated.

parth-20-07 added 2 commits October 16, 2025 15:29

feat: add num_enum crate to implement enum<>number conversion

4ddaf2f

- uses procedural macros to use the determinant of enum to convert number<>enum Signed-off-by: Parth Patel <parth.pmech@gmail.com>

feat: implement and test num_enum macro for RoutineControlSubfunction

e2055b5

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

parth-20-07 marked this pull request as ready for review October 20, 2025 16:47

parth-20-07 requested a review from Copilot October 20, 2025 16:47

Copilot AI reviewed Oct 20, 2025

View reviewed changes

parth-20-07 requested a review from Copilot October 20, 2025 16:51

Copilot AI reviewed Oct 20, 2025

View reviewed changes

parth-20-07 requested review from kymikoloco and zheylmun October 20, 2025 18:38

parth-20-07 self-assigned this Oct 20, 2025

parth-20-07 added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 20, 2025

parth-20-07 added 4 commits October 20, 2025 18:26

feat: implement const based descriminant for non-unit enum types

1e95d14

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

style(clippy): fix clippy pedantic suggestions

ea9044e

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

style(fmt): sort derives in alphabetical order

7d6ef40

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

fix(spell): correct spelling mistake

bcbec50

Signed-off-by: Parth Patel <parth.pmech@gmail.com>

parth-20-07 force-pushed the feat/constant-id-num-enum branch from b9f0a86 to bcbec50 Compare October 20, 2025 22:26

kymikoloco suggested changes Oct 21, 2025

View reviewed changes

kymikoloco reviewed Oct 21, 2025

View reviewed changes

parth-20-07 closed this Nov 4, 2025

-            Self::USER_DEF_MEMORY_DTC_EXT_DATA_RECORD_BY_DTC_NUMBER => todo!(),
+            Self::USER_DEF_MEMORY_DTC_EXT_DATA_RECORD_BY_DTC_NUMBER => {
+                Ok(Some(Self::UserDefMemoryDTCExtDataRecordByDTCNumberList(
+                    UserDefMemoryDTCExtDataRecordByDTCNumRecord::decode(reader)?.unwrap(),
+                )))
+            }

Conversation

parth-20-07 commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

kymikoloco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parth-20-07 commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

parth-20-07 commented Oct 17, 2025 •

edited

Loading

codecov Bot commented Oct 17, 2025 •

edited

Loading