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.

Sign up for GitHub

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

app: boards: add nucleo_h745zi_q support #31

Closed
wants to merge 1 commit into from
Closed

app: boards: add nucleo_h745zi_q support #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions app/boards/nucleo_h745zi_q.overlay
View file
Open in desktop
KozhinovAlexander marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright (c) 2024 Alexander Kozhinov <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/dt-bindings/gpio/gpio.h>

/ {
chosen {
zephyr,canbus = &fdcan1;
};
Comment on lines +10 to +12
Copy link
Member

Choose a reason for hiding this comment

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

This needs to go into Zephyr. see zephyrproject-rtos/zephyr#82640


cannectivity: cannectivity {
compatible = "cannectivity";
timestamp-counter = <&counter2>;

channel0 {
compatible = "cannectivity-channel";
can-controller = <&fdcan1>;
state-gpios = <&gpiob 0 GPIO_ACTIVE_HIGH>;
activity-gpios = <&gpioe 1 GPIO_ACTIVE_HIGH>;
Copy link
Member

Choose a reason for hiding this comment

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

So all this PR is really adding is support for a separate activity LED on this board (which does not have a designated CAN activity LED)? If that is all, I see no reason to pull it in, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True - the changes are very small. But they are needed to make CANnectivity running.
On one hand without this changes one needs to do them each time the board shall run CANNectivity.
On the other hand the introduced board is still a development board without specific CANnectivity functionality, therefore supporting CANnectivity needs this overlay.

Copy link
Member

Choose a reason for hiding this comment

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

True - the changes are very small. But they are needed to make CANnectivity running.

Needed how?

On one hand without this changes one needs to do them each time the board shall run CANNectivity. On the other hand the introduced board is still a development board without specific CANnectivity functionality, therefore supporting CANnectivity needs this overlay.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand you right the lines 14 to 22 are not needed and CANnectivity will work with this board just right from Zephyr main repository?

UPDATE: In my opinion this is alone the reason to add the overlay of the board to the project.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

If I understand you right the lines 14 to 22 are not needed and CANnectivity will work with this board just right from Zephyr main repository?

None of the lines in this PR (except the bugfix for the missing zephyr,canbus chosen, which needs to go into Zephyr) are needed for running CANnectivity on this board (have you tried)?

The addition of a separate activity LED is not really needed, as activity will be indicated on the state LED in it's absence. Separate activity LEDs are great for boards with dedicated CAN activity LEDs (e.g. boards intended for use as USB to CAN adapters).

The only addition that could warrant an overlay is really the added hardware timestamp counter, but I'm reluctant to start adding overlays for generic development boards only to add hardware timestamp support, a feature likely not many will require when using CANnectivity on a generic development board (or even know how to activate). These overlays will need to be maintained, tested - otherwise they will bitrot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
On the other side I see problems with development boards in general - they are designed to cover as much applications as possible, therefore hardware conflicts are unavoidable.
Sure, such conflicts may be solved by overlays, but the board is will still stay a development board and not a productive one.
I would suggest rejecting development board in general within CANnectivity.

};
};
};

&zephyr_udc0 {
gs_usb0: gs_usb0 {
compatible = "gs_usb";
};
};

&fdcan1 {
/* Allocate half of the M_CAN message RAM to FDCAN1 */
reg = <0x4000a000 0x400>, <0x4000ac00 0x1400>;
reg-names = "m_can", "message_ram";
Comment on lines +34 to +36
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Why half?

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will change to 10KiBi

bosch,mram-cfg = <0x0 1 1 10 10 0 10 10>;
pinctrl-0 = <&fdcan1_rx_pb8 &fdcan1_tx_pb9>;
Copy link
Member

Choose a reason for hiding this comment

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

How come this is changed from the board defaults of pinctrl-0 = <&fdcan1_rx_pd0 &fdcan1_tx_pd1>;?

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is only one shield, which pass perfectly to the connector on nucleo_h745 board: https://www.waveshare.com/rs485-can-shield.htm and it needs this pinout.
The pd0 and pd1 pinse are default pins nucleo allocates for CAN accroding to their dicumentation.
Therefore the change is needed.

Copy link
Member

Choose a reason for hiding this comment

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

But just because you need to use this particular board with that particular shield does not make it a generic configuration suitable for other users of CANnectivity. Your change makes the configuration incompatible with what others expect from the board schematic.

I suggest keeping this board + shield configuration in your own repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But just because you need to use this particular board with that particular shield does not make it a generic configuration suitable for other users of CANnectivity. Your change makes the configuration incompatible with what others expect from the board schematic.

I suggest keeping this board + shield configuration in your own repository.

It terms of CAN-L and CAN-H pins I may agree, but what about LEDs? see conversation: #31 (comment)

status = "okay";
};

&timers2 {
st,prescaler = <239>;
counter2: counter {
status = "okay";
};
};

&arduino_i2c {
status = "disabled";
};

&arduino_serial {
status = "disabled";
};
Comment on lines +48 to +55
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed.

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 will drop it. Need to test first.