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 IndustryGroup, DeviceClass and FunctionCode #22

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Thom-de-Jong
Copy link
Member

Instead of working with the u8 values, you now use enum types.

Also added the NameFilter concept from AgIsoStack++

88 => FunctionCode::ElectricPowerConverter,
89 => FunctionCode::SupplyEquipmentCommunicationController,
90 => FunctionCode::VehicleAdapterCommunicationController,
128 => FunctionCode::Reserved,
Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing a number of functions, like maybe this is just IG 0? Like for function 128, that's also "Rate Controller" for some IG/Device Class combinations.

image

I am not sure how complete we want to go, but it seems like we probably want at least all of IG 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed a lot and i'm strugeling a bit finding a nice way to represent all of it.

At the moment it is just IG0 as you suspected.

@@ -0,0 +1,1280 @@
pub mod reader;
Copy link
Member

Choose a reason for hiding this comment

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

Might want to sequence this PR after the Object pool one #23 since the object pool files are in this one as well

raw_name: u64,
}

impl NAME {
Copy link
Member

Choose a reason for hiding this comment

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

Idiomatically is it more "normal" in Rust to put this kind of thing in mod.rs rather than its own file? I'm just trying to figure out if name.rs was an abnormal thing to do over this where NAME's implementation is in mod.rs

Copy link
Member

Choose a reason for hiding this comment

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

My personal preference is to put module structure in mod.rs and lib.rs files, where structure is

  • module names
  • module visibility
  • symbol visibility

and to leave the implementation of those symbols in separate files. My opinion is mostly based on having the ability to do so, which is a breath of fresh air coming from C++ and Python 😆. I'm unsure if my perceived benefits of "better understanding the structure of the module" is actually realized in practice.

This is not an opinion it seems the open-source Rust community shares, based on digging through various open source Rust crates.

So, my short answer is: It's normal and fine to put the implementations wherever - as long as it results in a sane public-facing API.

Copy link
Member Author

Choose a reason for hiding this comment

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

In rust it does not really mater but as you pointed out, putting the implementation insize of a file called name.rs would make the structure more clear.

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