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

Implement at6561 can trx driver #20480

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

firas-hamdi
Copy link
Contributor

Contribution description

Write a driver for the AT6561 CAN transceiver built on the CAN generic transceiver interface can_trx.

Testing procedure

The driver is tested with tests/sys/can_trx application. Hardware used for testing is the same54-xpro devBoard.

@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers labels Mar 19, 2024
@firas-hamdi firas-hamdi force-pushed the feat/implement_at6561_can_trx_driver branch 2 times, most recently from d76c88d to 398a1e2 Compare March 19, 2024 08:42
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Mar 19, 2024
@firas-hamdi firas-hamdi changed the title Feat/implement at6561 can trx driver Implement at6561 can trx driver Mar 19, 2024
@firas-hamdi firas-hamdi force-pushed the feat/implement_at6561_can_trx_driver branch from 398a1e2 to ee8da05 Compare March 19, 2024 08:59
@github-actions github-actions bot removed the Area: tools Area: Supplementary tools label Mar 19, 2024
@firas-hamdi firas-hamdi force-pushed the feat/implement_at6561_can_trx_driver branch 2 times, most recently from 52360c7 to 5aca0e3 Compare March 19, 2024 09:18
@firas-hamdi firas-hamdi force-pushed the feat/implement_at6561_can_trx_driver branch from 5aca0e3 to ccd7fec Compare March 19, 2024 09:23
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

wouldn't a generic can_8pin_transceiver driver be a better option?

I don't know why there is a tja1042-driver in the first place, But this one certainly fits the intention of the tja1042-driver author to keep every can trx driver seperate.

? @vincent-d might know ?

Comment on lines +36 to +37
gpio_t stby_pin; /**< AT6561 standby pin */
can_trx_t trx; /**< AT6561 interface */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
gpio_t stby_pin; /**< AT6561 standby pin */
can_trx_t trx; /**< AT6561 interface */
can_trx_t trx; /**< AT6561 interface */
gpio_t stby_pin; /**< AT6561 standby pin */

please swap to keep consistent with the other cantrx_drivers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no problem with swapping them, but I don't like the usage of can_trx_t *trx = &at6561 as other drivers are used in the test app tests/sys/can_trx (I find it very error prone and it limits the freedom to define structure members)

Copy link
Contributor

Choose a reason for hiding this comment

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

just swap -- so it won't fail if someone does it like can_trx_t *trx = &at6561 as they might if they looked at the other drivers ( make it more simmilar)

@firas-hamdi
Copy link
Contributor Author

wouldn't a generic can_8pin_transceiver driver be a better option?

I don't know why there is a tja1042-driver in the first place, But this one certainly fits the intention of the tja1042-driver author to keep every can trx driver seperate.

? @vincent-d might know ?

I agree with that partially. We have already the can_trx as a generic transceiver driver, and we can use pseudo-modules on top of that to define different CAN transceivers.
But maybe the intention is to keep one driver for every CAN transceiver because there are two CAN transceiver drivers implemented separately

@benpicco benpicco requested a review from dylad March 19, 2024 12:29
@benpicco
Copy link
Contributor

You could have just used tja1042, as far as I can tell it is identical to this driver.

@firas-hamdi
Copy link
Contributor Author

firas-hamdi commented Mar 19, 2024

You could have just used tja1042, as far as I can tell it is identical to this driver.

I could yes, they are identical with the slight difference that at6561 does not provide the silent mode. But as I found different CAN transceiver drivers I though about adding a new one.

@benpicco
Copy link
Contributor

they are identical with the slight difference that at6561 does not provide the silent mode

As far as I can tell tja1042 doesn't provide silent mode either.

@firas-hamdi
Copy link
Contributor Author

they are identical with the slight difference that at6561 does not provide the silent mode

As far as I can tell tja1042 doesn't provide silent mode either.

You're right indeed. I just checked the driver and saw that the silent mode is same as normal mode configuration-wise.
But still the question: what if someone wants to use the AT6561 and does not catch the TJA1042? It's for this reason that I would prefer to have a driver for AT6561 (that can be extended with AT6560 features), or even better have a generic CAN transceiver with standby pin supporting both transceivers

@benpicco
Copy link
Contributor

We can rename TJA1042 to something more generic.

@kfessel
Copy link
Contributor

kfessel commented Mar 20, 2024

@firas-hamdi
drivers/at6561: driver for AT6561 CAN transceiver
fcb97bc
@firas-hamdi
tests/sys/can_trx: add AT6561 CAN transceiver

this is enough to convey all information ( just a tip on how to shorten commit messages) the area of work does not need to be repeated in the description

@firas-hamdi
Copy link
Contributor Author

We can rename TJA1042 to something more generic.

Should we classify the CAN transceivers according to how many modes they provide and we generate a naming from that?

@kfessel
Copy link
Contributor

kfessel commented Mar 21, 2024

We can rename TJA1042 to something more generic.

Should we classify the CAN transceivers according to how many modes they provide and we generate a naming from that?

most can-trx are 8 pin with one control pin the modes that set the pin can be put into a bitmask then set_mode function has just has to check if the bit-mask says it is has that bit set or not

@firas-hamdi
Copy link
Contributor Author

We can rename TJA1042 to something more generic.

Should we classify the CAN transceivers according to how many modes they provide and we generate a naming from that?

most can-trx are 8 pin with one control pin the modes that set the pin can be put into a bitmask then set_mode function has just has to check if the bit-mask says it is has that bit set or not

There are some CAN transceivers providing two control pins (ncv7356), should these be included too to the generic driver?

@kfessel
Copy link
Contributor

kfessel commented Mar 21, 2024

We can rename TJA1042 to something more generic.

Should we classify the CAN transceivers according to how many modes they provide and we generate a naming from that?

most can-trx are 8 pin with one control pin the modes that set the pin can be put into a bitmask then set_mode function has just has to check if the bit-mask says it is has that bit set or not

There are some CAN transceivers providing two control pins (ncv7356), should these be included too to the generic driver?

seems like a secial case (single wire can) that we better might not cover in our generic can-8pin-trx-driver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: tests Area: tests and testing framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants