Skip to content

Conversation

@Dalachowsky
Copy link

Describe your changes

This PR moves -20 messages from message/ to message_d20/ to prepare for -2 and DIN implementation. During the working group meeting today i have proposed to merge this to main early, because this introduces a lot of changes. Without this change, rebasing the -2 and DIN branches during development would be unnecessarily complex.

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

In preparation for DIN and -2 Support.
This is to allow putting -2 messages inside message_d2
and ensure consistency.

Signed-off-by: Kacper Dalach <kdalach@elektrometal.com.pl>
Copy link
Member

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

However, I would take a different approach and not blindly move all -20 messages.
For a start, it would be sufficient to add an additional folder for din and iso2:

include/iso15118/message
├── [...].hpp
├── din/
├── iso2/
└── [...].hpp

This way, you can work independently of the main and I have a little more time to define the folder structure.

Since implementation is also happening in parallel on the D20 EV side, the folder structure should be well thought out.

As already mentioned yesterday in the WG, this repo is currently being moved to everest-core. Once that has happened, this repo will be archived. Therefore, future PRs should also be created in everest-core.

As also discussed in the WG, I will create a GitHub project for the DIN & ISO-2 implementation and explain what needs to be done in the issues. I will also post a short explanation in the chat in zulip.

For this reason, I would not accept this PR for the time being.

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