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

Review-ready driver #6

Closed
wants to merge 40 commits into from
Closed

Review-ready driver #6

wants to merge 40 commits into from

Conversation

noxuz
Copy link

@noxuz noxuz commented Mar 24, 2020

No description provided.

Copy link
Contributor

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

So, I got this working after fixing the ISR attributes (see my other comments). Let's start by fixing the ISRs and verifying that your demo still works. You also need to provide a .cpp file since this is a concrete driver. Having the static linkage in a header will cause all sorts of neat problems so don't do that. Provide the class definitions in an hpp but put the implementation, ISR definitions, and any statically defined storage in a cpp file.

Once we have these two changes please update this review and I'll pick up from there.

nxp_s32k/libuavcan/s32k_libuavcan.hpp Outdated Show resolved Hide resolved
nxp_s32k/libuavcan/s32k_libuavcan.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

See comment below: I provided a sanity test you can build on if you choose to.

nxp_s32k/libuavcan/driver/include/s32k_libuavcan.hpp Outdated Show resolved Hide resolved
nxp_s32k/libuavcan/driver/include/s32k_libuavcan.hpp Outdated Show resolved Hide resolved
nxp_s32k/libuavcan/driver/include/s32k_libuavcan.hpp Outdated Show resolved Hide resolved
nxp_s32k/libuavcan/driver/include/s32k_libuavcan.hpp Outdated Show resolved Hide resolved
/* SysClock initialization for feeding 80Mhz to FlexCAN */

/* System Oscillator (SOSC) initialization for 8Mhz external crystal */
SCG->SOSCCSR &= ~SCG_SOSCCSR_LK_MASK; /* Ensure the register is unlocked */
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reasonable use of this driver will not tolerate it setting the system oscillator. You can document and respond to SCG settings and provide these settings in your examples but you cannot set them here or anywhere else in a media layer implementation.

While we can argue that the driver should setup clocks used exclusively by the CAN peripherals, core clock tree configuration must live outside of this code.

Copy link
Author

Choose a reason for hiding this comment

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

From page 572 of the reference manual:
FlexCAN_clocks

The peripheral has only 2 clock sources available, SYS_CLK is the current configured one, which is synchronous, it is also the ony one that allows for descent bitrates, the other source SOSCDIV2_CLK, which is in fact asynchronous (comes from the external crystal currently set to 8Mhz) would reduce the current bitrates of 4Mbit/s in data and 1Mbit/s in nominal in at least tenfold

Copy link
Contributor

Choose a reason for hiding this comment

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

comes from the external crystal currently set to 8Mhz

This, for example, is board-dependent. Not all boards have the exact same external parts and may have other peripherals that require the clock tree to be setup differently (e.g. using an RC oscillator instead of an XOSC or using an XOSC ). This is one of the reasons that any non-trivial application uses an OS; peripherals have to share system resources and that requires compromises. For a bare-metal driver we can document the requirements and effects various clock rates will cause but you can't have one driver own the master clock on a system.

Try this to help you understand: use the S32 design studio to create a new project from the CAN-FD example. You'll see the code generated includes files that initialize the clocks and separate files that initialize the driver.

libuavcan only wants to provide the driver part. We document and check clock requirements but we don't set them.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that a driver shouldn't touch an important setting such as the clock speed that the CPU is running from.
The mcu also has a couple of internal clock sources that are independent from the CPU, and the external oscillator:

From the reference manual page 558
imagen

FIRCDIVx_CLK and SIRCDIVx_CLK are meant to feed peripherals that their setup must be independent from a selected core clock configuration, as Timers or UART for example.

But these are not available to the FlexCAN as mentioned before, the only alternative is to feed from SOCDIV2_CLK and document that a 8Mhz crystal is assumed, which is the one that both the EVB's and the UCANS32K146 reference design boards have, with the lower bitrate tradeoff.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can document this requirement but should check it as well in the manager returning an error result if the clock is not set as we require. We'll want to improve on this in the future providing an ability to set different clock speeds for the bus and handling different master clock speeds. We'll capture that work as an issue in this repo though.

Copy link
Author

Choose a reason for hiding this comment

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

I could setup for example 4 different predefined and validated bitrate speed profiles, and set a tunable constant such as the already used FIFO depth that chooses from these.

Copy link

Choose a reason for hiding this comment

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

@noxuz I've ported the S32 design studio bit timing calculation to the NuttX FlexCAN driver, which calculates the bit timing for any bitrate and clock configuration, the only thing is that you've to optimize is the sample point.

https://github.com/PetervdPerk-NXP/incubator-nuttx/blob/8bde0f30edbea157c93d2a6de12890426924668d/arch/arm/src/s32k1xx/s32k1xx_flexcan.c#L403-L505

@thirtytwobits
Copy link
Contributor

thirtytwobits commented Mar 29, 2020

I've setup a sanity test that ensures this driver compiles. Please cherrypick this change into your repo and rebase your PR on it:

thirtytwobits@3f1fd57

This change doesn't include all of the comments I have here so far (e.g. the class renaming and namespace adjustments). With a bit more work you'd be able to write a little software simulator that would allow you to instantiate the InterfaceGroup objects as well but I won't hold you to that for this PR.

…d enhanced its description, private to public inheritance fix, moved S32K namespace unctions from header, macro guard in build config fix.
@noxuz
Copy link
Author

noxuz commented Mar 29, 2020

I the meantime I implement the next changes and check the vscode tool, I have updated the demo project with the build configurations enabled and the latest driver also. For verifying g++ compilation. Link

@thirtytwobits
Copy link
Contributor

Okay. We're getting there structurally. Just merge in my test code and we can start digging into the guts of the driver.

Keep it up!

@thirtytwobits
Copy link
Contributor

Oh, one more thing I forgot, get rid of build_config.hpp. it's unnecessary. We don't need more than the two files here. Just move:

/**
 * Macro for additional configuration needed when using a TJA1044 transceiver, which is used
 * in NXP's UCANS32K146 board, set to 0 when using EVB's or other boards.
 */
#ifndef UAVCAN_NODE_BOARD_USED
#    define UAVCAN_NODE_BOARD_USED 1
#endif

/**
 * Include desired target S32K14x memory map header file dependency,
 * defaults to S32K146 from NXP's UCANS32K146 board
 */
#include "S32K146.h"

into the canfd.hpp header.

S32K/libuavcan/media/S32K/canfd.hpp Outdated Show resolved Hide resolved
S32K/libuavcan/media/S32K/build_config.hpp Outdated Show resolved Hide resolved
S32K/libuavcan/media/S32K/canfd.hpp Outdated Show resolved Hide resolved
S32K/canfd.cpp Outdated Show resolved Hide resolved
S32K/README.md Outdated Show resolved Hide resolved
S32K/libuavcan/media/S32K/canfd.hpp Outdated Show resolved Hide resolved
S32K/libuavcan/media/S32K/canfd.hpp Outdated Show resolved Hide resolved
S32K/libuavcan/media/S32K/canfd.hpp Outdated Show resolved Hide resolved
@@ -0,0 +1,904 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the reason I had a folder that contained the cpp and the includes in my example change is we want to allow for git subtrees to be pulled that exclude all the extraneous test code. Can you introduce something like "module" to allow for this? So the top level tree would be:

module 
    |
    + canfd.cpp
    + libuavcan/media/S32K/canfd.hpp
test
    |
    + CMakeLists.txt
...

You also need to update CMakeLists.txt with the correct path the .cpp and include

Copy link
Author

Choose a reason for hiding this comment

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

Implemented at noxuz@408beca

@pavel-kirienko
Copy link
Member

@thirtytwobits should I close this?

@thirtytwobits
Copy link
Contributor

Per discussion at last week's UAVCAN dev call we're going to close this and finish this work in libuavcan's verification driver.

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

5 participants