app: boards: add nucleo_h745zi_q support #31
Conversation
KozhinovAlexander
force-pushed
the
nucleo_h745zi_q_board
branch
from
aa3249b to
6a73f6f
Compare
KozhinovAlexander
force-pushed
the
nucleo_h745zi_q_board
branch
6 times, most recently
from
de3c2c8 to
1434058
Compare
|
@henrikbrixandersen do you plan to take a look here? |
Signed-off-by: Alexander Kozhinov <[email protected]>
KozhinovAlexander
force-pushed
the
nucleo_h745zi_q_board
branch
from
1434058 to
0030d4a
Compare
| chosen { | ||
| zephyr,canbus = &fdcan1; | ||
| }; |
There was a problem hiding this comment.
This needs to go into Zephyr. see zephyrproject-rtos/zephyr#82640
| /* Allocate half of the M_CAN message RAM to FDCAN1 */ | ||
| reg = <0x4000a000 0x400>, <0x4000ac00 0x1400>; | ||
| reg-names = "m_can", "message_ram"; |
There was a problem hiding this comment.
Why is this needed? Why half?
There was a problem hiding this comment.
Will change to 10KiBi
| reg = <0x4000a000 0x400>, <0x4000ac00 0x1400>; | ||
| reg-names = "m_can", "message_ram"; | ||
| bosch,mram-cfg = <0x0 1 1 10 10 0 10 10>; | ||
| pinctrl-0 = <&fdcan1_rx_pb8 &fdcan1_tx_pb9>; |
There was a problem hiding this comment.
How come this is changed from the board defaults of pinctrl-0 = <&fdcan1_rx_pd0 &fdcan1_tx_pd1>;?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
|
||
| &arduino_i2c { | ||
| status = "disabled"; | ||
| }; | ||
|
|
||
| &arduino_serial { | ||
| status = "disabled"; | ||
| }; |
There was a problem hiding this comment.
I will drop it. Need to test first.
| compatible = "cannectivity-channel"; | ||
| can-controller = <&fdcan1>; | ||
| state-gpios = <&gpiob 0 GPIO_ACTIVE_HIGH>; | ||
| activity-gpios = <&gpioe 1 GPIO_ACTIVE_HIGH>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Closed. Rejecting development boards approach in general shall be followed. |
Add support nor NUCLEO-H745ZI_Q board.