Skip to content

can: add ncv7356 SW transceiver driver#6927

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
OTAkeys:pr/ncv7356
Aug 22, 2019
Merged

can: add ncv7356 SW transceiver driver#6927
aabadie merged 2 commits intoRIOT-OS:masterfrom
OTAkeys:pr/ncv7356

Conversation

@vincent-d
Copy link
Copy Markdown
Member

@vincent-d vincent-d commented Apr 18, 2017

Follow up, based on #5793

This is the NCV7356 (https://www.onsemi.com/pub/Collateral/NCV7356-D.PDF) single wire CAN transceiver driver.

@miri64 miri64 requested a review from aabadie May 3, 2017 12:10
@miri64 miri64 added the Area: drivers Area: Device drivers label May 3, 2017
@aabadie aabadie added this to the Release 2017.07 milestone Jun 23, 2017
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 23, 2017

needs rebase

@aabadie aabadie mentioned this pull request Jun 26, 2017
Merged
@vincent-d vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 18, 2018
@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented Jan 18, 2018

Rebased since #6925 has been merged

@vincent-d vincent-d removed this from the Release 2018.04 milestone Apr 11, 2018
aabadie
aabadie requested changes Apr 11, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Looks good but I can't test since I don't have the hardware. See my style comments below.

drivers/Makefile.dep Outdated
FEATURES_REQUIRED += periph_gpio
endif

ifneq (,$(filter nvc7356,$(USEMODULE)))
Copy link
Copy Markdown
Contributor

@aabadie aabadie Apr 11, 2018

Choose a reason for hiding this comment

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

Please rebase and follow alphabetical order

drivers/include/ncv7356.h Outdated

/**
* @defgroup drivers_ncv7356 NCV7356
* @ingroup drivers
Copy link
Copy Markdown
Contributor

@aabadie aabadie Apr 11, 2018

Choose a reason for hiding this comment

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

Not sure, but is there a drivers_can group ?

drivers/ncv7356/ncv7356.c
int ret;

switch (mode) {
case TRX_NORMAL_MODE:
Copy link
Copy Markdown
Contributor

@aabadie aabadie Apr 11, 2018

Choose a reason for hiding this comment

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

case should be indented

Copy link
Copy Markdown
Member Author

@vincent-d vincent-d Aug 21, 2019

Choose a reason for hiding this comment

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

there are some places where cases are not indented, so I guess we can keep it like that.

tests/can_trx/Makefile Outdated
endif

ifneq (,$(filter ncv7356,$(TRX_TO_BUILD)))
USEMODULE += ncv7356
Copy link
Copy Markdown
Contributor

@aabadie aabadie Apr 11, 2018

Choose a reason for hiding this comment

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

use 2 spaces indent in makefile

@tcschmidt
Copy link
Copy Markdown
Member

tcschmidt commented Jun 2, 2018

Who has hardware to test this? @PeterKietzmann maybe we ask @Musteblume ?

@stale
Copy link
Copy Markdown

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 21, 2019
@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented Aug 21, 2019

@aabadie I addressed your comments.

@aabadie aabadie added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Aug 21, 2019
aabadie
aabadie reviewed Aug 21, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

This still looks good. I checked the set mode function against the datasheet and the implementation is correct.
Unfortunately I cannot test but if it's fine for another maintainer (@PeterKietzmann ?) to give an untested ACK to this PR then it can be merged.

aabadie
aabadie approved these changes Aug 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

I compared this PR with the tja1042 and they are very similar (in term of design), the one in this PR just provides more modes (with 2 configuration pins). I don't see why one will work the other not.
I take on me to give an un-tested ACK and finally merge this :)

@aabadie aabadie merged commit 349b83c into RIOT-OS:master Aug 22, 2019
@jcarrano
Copy link
Copy Markdown
Contributor

jcarrano commented Aug 23, 2019

un-tested ACK

Could we not do this again, please?

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
@JannesVolkens
Copy link
Copy Markdown
Contributor

JannesVolkens commented Oct 25, 2019

@vincent-d Where can I find the initialization of mode0_pin and mode1_pin? Or am I missing something?

@vincent-d vincent-d deleted the pr/ncv7356 branch October 25, 2019 12:51
@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented Oct 28, 2019

@JannesVolkens the pins are initialized in the init function, then the initial value is set with a call to ncv7356_trx_set_mode().

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 28, 2019

I guess the question was more about where the pins are configured. Usually, drivers provide a params file with a default config. This is not the case here.

@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented Oct 28, 2019

oops, sorry for the misunderstanding.

Indeed, one needs to initialize an ncv7356_trx_t struct and pass it to the driver.

I have this kind of code in my board config:

ncv7356_trx_t trx_sw_can = {
    .trx        = {
        .driver = &ncv7356_driver,
    },
    .mode0_pin  = CAN_MODE0_PIN,
    .mode1_pin  = CAN_MODE1_PIN,
};

Then in the can_params.h file I have something like this:

static const candev_params_t candev_params[] = {
    {
        .name = "can1",
        .trx = (can_trx_t *)&trx_sw_can,
    },
}

So the transceiver is used by the CAN stack automatically.

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 28, 2019

Thanks for sharing!

@JannesVolkens
Copy link
Copy Markdown
Contributor

JannesVolkens commented Nov 18, 2019

I tested the driver for the NCV7356 transceiver using the existing test class can_trx and was able to verify the actual setting and clearing of the two mode pins. I have attached a screenshot from a logic analyzer showing the two lines connected to MODE0 and MODE1, where Channel 0 is MODE0 and Channel 1 is MODE1. You can see the initialization of the device and setting of the sleep mode, high-speed mode, high voltage wake-up mode and normal mode in the given order. The other screenshot shows the terminal and the different printouts of the can_trx test class.

NCV7356TEST

NCV7356TESTING

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie approved these changes

Assignees

@aabadie aabadie

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

9 participants

@vincent-d @aabadie @tcschmidt @jcarrano @JannesVolkens @PeterKietzmann @miri64 @kb2ma @smlng