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

Jump to bottom

MKS CANable V2.0 #81366

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

MKS CANable V2.0 #81366

wants to merge 2 commits into from

Conversation

KozhinovAlexander
Copy link
Collaborator

add new board MKS CANable V2.0 Please refer to added documentation for more information.
The hardware of the board can be dound under: https://github.com/makerbase-mks/CANable-MKS

@KozhinovAlexander
Copy link
Collaborator Author

@henrikbrixandersen You will see an overlay for this board in CANnectivity later. ;) Thus please take a look too.

@KozhinovAlexander KozhinovAlexander force-pushed the mks_canable_v2 branch 2 times, most recently from ce12700 to b6f5079 Compare November 13, 2024 23:59
@KozhinovAlexander KozhinovAlexander requested review from erwango, decsny and galak and removed request for galak and decsny November 13, 2024 23:59
@KozhinovAlexander KozhinovAlexander changed the title Add new board MKS CANable V2.0 MKS CANable V2.0 Nov 14, 2024
nordicjm
nordicjm reviewed Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Board is OK, please fix ordering issue

@KozhinovAlexander
Copy link
Collaborator Author

Board is OK, please fix ordering issue

Thank you for you review. Great to see a feedback from hwv2 guy :) I'll do necessary changes today late night.

erwango
erwango requested changes Nov 14, 2024
View reviewed changes
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/mks_canable_v20.dts Outdated Show resolved Hide resolved
nordicjm
nordicjm reviewed Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Updates need squashing i.e. 2 commits

@KozhinovAlexander
Copy link
Collaborator Author

Updates need squashing i.e. 2 commits

Thank you. It is done now.

@nordicjm
Copy link
Collaborator

Updates need squashing i.e. 2 commits

Thank you. It is done now.

So the second vendor update commit needs squashing into the first vendor update commit and the second board commit needs squashing into the first board commit. Something like this:

git rebase -i 40cd35e56d43716bdbdeafafa6c356407121ca2e
  change first to edit, third to fixup, forth to drop
git cherry-pick ddf9bfdf02006c2dc4e2012ae5ad93a3a0fde584
git rebase --continue
git rebase -i 40cd35e56d43716bdbdeafafa6c356407121ca2e
  change second to fixup

Now you should have 2 commits

@KozhinovAlexander
Copy link
Collaborator Author

Updates need squashing i.e. 2 commits

Thank you. It is done now.

So the second vendor update commit needs squashing into the first vendor update commit and the second board commit needs squashing into the first board commit. Something like this:

git rebase -i 40cd35e56d43716bdbdeafafa6c356407121ca2e
  change first to edit, third to fixup, forth to drop
git cherry-pick ddf9bfdf02006c2dc4e2012ae5ad93a3a0fde584
git rebase --continue
git rebase -i 40cd35e56d43716bdbdeafafa6c356407121ca2e
  change second to fixup

Now you should have 2 commits

Got it. Will do.

nordicjm
nordicjm previously approved these changes Nov 18, 2024
View reviewed changes
henrikbrixandersen
henrikbrixandersen requested changes Nov 18, 2024
View reviewed changes
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

Given that upstream threats this as one of multiple board variants (see https://github.com/makerbase-mks/CANable-MKS), I think we might want to do the same (bu renaming the board folder etc.). Support for the V1.0 and the PRO editions can then be added later on. What do you think?

boards/makerbase/index.rst Show resolved Hide resolved
boards/makerbase/mks_canable_v20/board.yml Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/doc/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/mks_canable_v20.yaml Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/mks_canable_v20.yaml Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/mks_canable_v20_defconfig Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/support/openocd.cfg
Comment on lines +28 to +37
if {[version_compare $current_version "0.12.0"] >= 0} {
# OpenOCD version is 0.12.0 or newer
init
rtt setup 0x20000000 10000 "SEGGER RTT"
rtt start
rtt server start 9090 0
} else {
# OpenOCD version is older than 0.12.0
echo "Warn: Segger RTT is not supported for this OpenOCD version: $current_version"
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks very generic and should probably not just be set for one board. It could go into a common OpenOCD configuration file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean by common the configuration file delivered with OpenOCD itself?

Copy link
Member

@henrikbrixandersen henrikbrixandersen Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I meant boards/common/openocd.board.cmake. I would suggest submitting this in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant boards/common/openocd.board.cmake. I would suggest submitting this in a separate PR.

It is a good advice. Since it wil blow up the current scope and I can not test it for other boards - I would prefer to leave it here first. After merging this PR I would reallocate it to boards/common/openocd.board.cmake file. Or you can fell free to do so. I can support.

dts/bindings/vendor-prefixes.txt Show resolved Hide resolved
kartben
kartben reviewed Nov 24, 2024
View reviewed changes
boards/makerbase/index.rst Outdated Show resolved Hide resolved
boards/makerbase/mks_canable_v20/mks_canable_v20.yaml Outdated Show resolved Hide resolved
boards/makerbase/index.rst Show resolved Hide resolved
@KozhinovAlexander
dts: bindings: vendor-prefixes.txt
235dc53 
Add Makerbase Co., Ltd. board vendor

Signed-off-by: Alexander Kozhinov <[email protected]>
@KozhinovAlexander
boards: makerbase: mks_canable_v20: add new board MKS CANable V2.0
ec958c6 
A cheap and affordable stm32g4 based simple CAN and CAN-FD to usb
adapter board

Signed-off-by: Alexander Kozhinov <[email protected]>
Reviewed-by: Erwan Gouriou <[email protected]>
Reviewed-by: Henrik Brix Andersen <[email protected]>
@KozhinovAlexander
Copy link
Collaborator Author

Given that upstream threats this as one of multiple board variants (see https://github.com/makerbase-mks/CANable-MKS), I think we might want to do the same (bu renaming the board folder etc.). Support for the V1.0 and the PRO editions can then be added later on. What do you think?

There are a lot of differences betwee each version of the HW. To me their versioning seems more as a tagging of evolution and not of progress.

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

@decsny decsny Awaiting requested review from decsny

@galak galak Awaiting requested review from galak

@erwango erwango Awaiting requested review from erwango

@nordicjm nordicjm Awaiting requested review from nordicjm

@kartben kartben Awaiting requested review from kartben

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
area: Devicetree
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@KozhinovAlexander @nordicjm @kartben @henrikbrixandersen @erwango @zephyrbot