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 core address claiming logic #15

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Add core address claiming logic #15

merged 3 commits into from
Sep 25, 2023

Conversation

ad3154
Copy link
Member

@ad3154 ad3154 commented Aug 9, 2023

  • Added the basic address claiming logic.
  • Adds stub for CANMessage
  • Adds stubs for some network manger functionality
  • Needs still needs to be attached to the driver so it won't really do anything yet

Questions I have:

  • Is there a more idomatic way to do a finite state machine than match ?
  • Is there a setting we can adjust so the formatter doesn't make the entire state machine an unreadable blob of newlines?

@ad3154 ad3154 self-assigned this Aug 9, 2023
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

File Coverage Lines Branches
All files 26% 26% 0%
src/network_management/can_message.rs 0% 0% 0%
src/network_management/control_function.rs 13% 13% 0%
src/network_management/name.rs 55% 55% 0%
src/network_management/network_manager.rs 22% 22% 0%

Minimum allowed coverage is 0%

Generated by 🐒 cobertura-action against a4b8068

Copy link
Member

@GwnDaan GwnDaan left a comment

Choose a reason for hiding this comment

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

Looks promising 🔥 , for your questions:

  • Is there a more idomatic way to do a finite state machine than match ?

I googled around a bit, and like this approach quite a bit. It should also help with the next question.

  • Is there a setting we can adjust so the formatter doesn't make the entire state machine an unreadable blob of newlines?

Hmm, In my opinion refactoring the state machine into smaller concrete functions (for example as the state machine linked above, or by using early returns 😛) should be the first priority to battle this issue. Though I agree, it sometimes is unavoidable to have the longer line lengths and it may be increased just slightly (if there is an easy and standardized way to do so)

src/network_management/can_message.rs Show resolved Hide resolved
src/network_management/can_message.rs Outdated Show resolved Hide resolved
src/network_management/can_message.rs Outdated Show resolved Hide resolved
src/network_management/network_manager.rs Outdated Show resolved Hide resolved
src/network_management/network_manager.rs Outdated Show resolved Hide resolved
src/network_management/network_manager.rs Outdated Show resolved Hide resolved
@ad3154
Copy link
Member Author

ad3154 commented Aug 9, 2023

I googled around a bit, and like this approach quite a bit.

Oh, hey, that looks pretty nice... could tidy it up a fair bit. I'll experiment with that

src/network_management/network_manager.rs Outdated Show resolved Hide resolved
src/network_management/network_manager.rs Outdated Show resolved Hide resolved
src/network_management/can_message.rs Outdated Show resolved Hide resolved
@ad3154
Copy link
Member Author

ad3154 commented Aug 10, 2023

I googled around a bit, and like this approach quite a bit.

Oh, hey, that looks pretty nice... could tidy it up a fair bit. I'll experiment with that

Am I missing something? That approach doesn't seem to compile....
image

@GwnDaan
Copy link
Member

GwnDaan commented Aug 10, 2023

I googled around a bit, and like this approach quite a bit.

Oh, hey, that looks pretty nice... could tidy it up a fair bit. I'll experiment with that

Am I missing something? That approach doesn't seem to compile.... image

Welp I missed the most important part of the post:

In this post I instead want to focus on ✨ the future ✨. How could we plausibly represent this in Rust by extending the language. In the "future directions" section of the last post we wrote a state machine without the blinky lights. Using enum variants types (not yet a thing) combined with arbitrary self-types (also not yet a thing), we could write the following:

So that suggestion doesn't work hahah, sorry

@ad3154
Copy link
Member Author

ad3154 commented Aug 10, 2023

So that suggestion doesn't work hahah, sorry

Bummer, it was so clean and nice! Regardless, I'll still try and make it more self-contained by moving logic into AddressClaimingState.

@ad3154 ad3154 force-pushed the ca-adg/address-claim branch 2 times, most recently from 21b9296 to 480014b Compare August 10, 2023 22:52
@Thom-de-Jong
Copy link
Member

Thom-de-Jong commented Aug 11, 2023

Just want to add to this conversation.

State machines can be modeled in Rust using using enum's, as one would normally do in c++.
State machines however, really shine when combined with Rust's compile-time guarantees, making it impossible to program a transition to a state that we should not be in. (Typestate Programming)

The best example, that i know of, explaining this concept for state machines can be found here (Pretty State Machine Patterns in Rust )

Note however that this relatively requires a lot of code. Luckily most of it will be abstracted away by the compiler.
I would say, if we use Rust for its awesome, safe, compiler, we should build our software so the compiler will check for invalid states.

What do you think?

--edit:
This way you also don't need match statements

@GwnDaan
Copy link
Member

GwnDaan commented Aug 11, 2023

@Thom-de-Jong that sounds interesting! I think we should at least try it, though I'm also fine with doing at a later stage with a seperate PR if that's easier for @ad3154

Note however that this relatively requires a lot of code.

Personally that is fine with me, the large switch/match statements for some of the state machines we have over in AgIsoStack++ are hard to gasp IMO. So anything resulting is a somewhat smaller functions of code and especially with the "Rust's compile-time guarantees, making it impossible to program a transition to a state that we should not be in." I'm in for.

@ad3154
Copy link
Member Author

ad3154 commented Aug 11, 2023

Hmm, that does seem nice, but agreed it's a lot of code. I'd be fine with trying it but yeah maybe I can follow up this PR with a refactor. We have a ton of state machines in AgIsoStack++ so getting a nice pattern figured out will make our lives easier for other parts of the stack as well, so thanks for the suggestion!

@ad3154 ad3154 added the iso: network managment Related to the ISO-11783:5 standard label Aug 15, 2023
Added the basic address claiming logic.
Needs still needs to be attached to the driver.
Moved a lot of the address claim logic into the control_function file.
Cleaned up todos involving CanId encoding when creating address claim messages.
Removed some clippy allows.
Changed some manual bitshifting to use to_le_bytes.
Changed some raw integers to be "Pgn" "Address" and other types in driver.
Still WIP
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Patch coverage is 15.21% of modified lines.

Files Changed Coverage
src/driver/can_id.rs ø
src/network_management/can_message.rs 0.00%
src/network_management/control_function.rs 15.06%
src/network_management/network_manager.rs 16.43%

📢 Thoughts on this report? Let us know!.

…ager additions

Changed CFs to be stored differently.
Added a function to send a CAN message.
Added a way to create an ICF.
Started adding receive message processing.
@ad3154
Copy link
Member Author

ad3154 commented Sep 25, 2023

I think this is in a good enough state to merge now so we can keep making progress. Since last time, I rebased and updated all usages of NAME in each commit to match the latest implementation of NAME. Would be nice to get this in before #26, and will give us something I can test the driver layer with

@ad3154 ad3154 merged commit 44006ea into main Sep 25, 2023
7 of 9 checks passed
@ad3154 ad3154 deleted the ca-adg/address-claim branch September 25, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iso: network managment Related to the ISO-11783:5 standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants