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

CanInterface: add set_can_params method to set multiple parameters #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Urist-McGit
Copy link

This new method allows to set multiple CAN-specific parameters at once instead of over multiple netlink messages.

This is also required for some more obscure devices which might not accept some parameter configurations split over multiple netlink messages.

This new method allows to set multiple CAN-specific parameters at once
instead of over multiple netlink messages.

This is also required for some more obscure devices which might not
accept some parameter configurations split over multiple netlink
messages.
@fpagliughi
Copy link
Collaborator

This does seem like a good idea - to be able to set multiple parameters in a single call. But I'm wondering about maintaining another data structure so similar to InterfaceCanParams. Maybe, but I'm not totally convinced. Should we consider one structure and use a read/modify/write pattern similar to other Linux kernel API's? Or consider another way to structure the data? Shouldn't we also have a builder (pattern) for the parameters to be set?

But if we do go with what you suggest - a separate struct of all optional, settable parameters,

  • Did we get all of them?
  • Don't we want a matching getter?
  • Shouldn't we be able to initialize from InterfaceCanParams?
  • Should we reconsider the name? SetCanParams reads like a verb. And these are specifically the interface parameters. So, SettableInterfaceCanParams? Although that is a bit long.

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.

None yet

2 participants