X7Pro adds CAN driver #15348

CUAVcaijie · 2020-07-16T09:15:33Z

Because TIM5 of X7 is used for PWM, CAN uses TIM2

dagar · 2020-07-17T21:30:07Z

src/drivers/uavcan/CMakeLists.txt

@@ -46,6 +46,11 @@ if(CONFIG_ARCH_CHIP)
 	elseif(${CONFIG_ARCH_CHIP} MATCHES "stm32h7")
 		set(UAVCAN_DRIVER "stm32h7")
 		set(UAVCAN_TIMER 5) # The default timer is TIM5
+		if(CONFIG_CDCACM_VENDORSTR)
+			if(${CONFIG_CDCACM_VENDORSTR} MATCHES "CUAV")
+				set(UAVCAN_TIMER 2)


Looks good other than this part. The goal is to not have any vendor specific code outside of the board directory. We should add something to the board (boards/cuav/x7pro/default.cmake) that sets this.

Alternatively we could take a quick look at the other H7 boards to see if we can get away with changing the default timer on stm32h7.

Can it be added like this?

Yes that works for now.

Later it might make sense to move this and UAVCAN_INTERFACES to board_config.h and out of cmake entirely.

What do I need to do?

@dagar Hello, how is the progress?

@dagar Do I need to modify it?

dagar

Add a per board mechanism to set the UAVCAN timer or see if we can change the stm32h7 default.

ghulands · 2020-08-10T06:46:41Z

Hi @CUAVcaijie ,
I pulled in these changes and tested locally on the x7. I wasn't able to get it to connect to the Neo v2 Pro. Is this expected?

CUAVcaijie · 2020-08-10T07:09:18Z

Hi @CUAVcaijie ,
I pulled in these changes and tested locally on the x7. I wasn't able to get it to connect to the Neo v2 Pro. Is this expected?

@ghulands
Because Neo V2 Pro needs to assign node ID automatically, Px4 does not support it

CUAVcaijie · 2020-08-13T00:39:39Z

@dagar @davids5 Can you review it again?

davids5

As written this would break all the configurations. The approach should be to allow an override. If not all the cmake.xxxx files will have to set the UAVCAN_TIMER.

The other approach is to set UAVCAN_TIMER in the cmake.default, and to a test and set it if not not set in the 3 places you deleted it.

I like the approach with the override as the name says what it is dong.

davids5 · 2020-08-13T10:26:55Z

boards/cuav/x7pro/src/board_config.h

@@ -153,8 +153,11 @@
 /* High-resolution timer */
 #define HRT_TIMER               3  /* use timer3 for the HRT */
 #define HRT_TIMER_CHANNEL       3  /* use capture/compare channel 3 */
-#define STM32_RCC_APB1ENR  STM32_RCC_APB1LENR
-#define RCC_APB1ENR_TIM3EN RCC_APB1LENR_TIM3EN
+#define STM32_RCC_APB1ENR  	STM32_RCC_APB1LENR


These 5 lines do not belong in board_config.h They belongs in the timer driver (preferred) or the micro_hal

Thanks for your reply,but could you tell me the exact file i need to add these 5 lines? I think mocro_hal is a common file ,should i change it ? Look forward to your reply.

How about the H7 micro_hal? platforms/nuttx/src/px4/stm/stm32h7/include/px4_arch/micro_hal.h

Then build all the H7 targets and fix them as well.

davids5 · 2020-08-13T10:29:23Z

cmake/px4_add_board.cmake

@@ -221,6 +222,10 @@ function(px4_add_board)
 		set(config_uavcan_num_ifaces ${UAVCAN_INTERFACES} CACHE INTERNAL "UAVCAN interfaces" FORCE)
 	endif()

+	if(UAVCAN_TIMER)
+		set(config_uavcan_timer_num_ifaces ${UAVCAN_TIMER} CACHE INTERNAL "UAVCAN TIMER" FORCE)


Suggested change

set(config_uavcan_timer_num_ifaces ${UAVCAN_TIMER} CACHE INTERNAL "UAVCAN TIMER" FORCE)

set(config_uavcan_timer_override ${UAVCAN_TIMER_OVERRIDE} CACHE INTERNAL "UAVCAN TIMER OVERRIDE FORCE)

davids5 · 2020-08-13T10:29:53Z

src/drivers/uavcan/CMakeLists.txt

@@ -65,7 +62,7 @@ string(TOUPPER "${UAVCAN_DRIVER}" UAVCAN_DRIVER_UPPER)
 add_definitions(
 	-DUAVCAN_${UAVCAN_DRIVER_UPPER}_${OS_UPPER}=1
 	-DUAVCAN_${UAVCAN_DRIVER_UPPER}_NUM_IFACES=${config_uavcan_num_ifaces}
-	-DUAVCAN_${UAVCAN_DRIVER_UPPER}_TIMER_NUMBER=${UAVCAN_TIMER}
+	-DUAVCAN_${UAVCAN_DRIVER_UPPER}_TIMER_NUMBER=${config_uavcan_timer_num_ifaces}


Suggested change

-DUAVCAN_${UAVCAN_DRIVER_UPPER}_TIMER_NUMBER=${config_uavcan_timer_num_ifaces}

-DUAVCAN_${UAVCAN_DRIVER_UPPER}_TIMER_NUMBER=${UAVCAN_TIMER}

davids5 · 2020-08-13T10:34:54Z

src/drivers/uavcan/CMakeLists.txt

 	elseif(${CONFIG_ARCH_CHIP} MATCHES "stm32")
 		set(UAVCAN_DRIVER "stm32")
-		set(UAVCAN_TIMER 5) # The default timer is TIM5
 	endif()
 endif()



this should be

if (DEFINED config_uavcan_timer_override) set (UAVCAN_TIMER ${config_uavcan_timer_override}) endif()

davids5 · 2020-08-13T10:35:32Z

src/drivers/uavcan/CMakeLists.txt

@@ -42,13 +42,10 @@ set(UAVCAN_PLATFORM "generic")
 if(CONFIG_ARCH_CHIP)
 	if(${CONFIG_ARCH_CHIP} MATCHES "kinetis")
 		set(UAVCAN_DRIVER "kinetis")
-		set(UAVCAN_TIMER 1)


this will break all the drivers. All of this needs to stay./

davids5 · 2020-08-13T10:36:25Z

cmake/px4_add_board.cmake

@@ -142,6 +142,7 @@ function(px4_add_board)
 			IO
 			BOOTLOADER
 			UAVCAN_INTERFACES
+			UAVCAN_TIMER


Suggested change

UAVCAN_TIMER

UAVCAN_TIMER_OVERRIDE

davids5 · 2020-08-13T10:37:01Z

cmake/px4_add_board.cmake

@@ -221,6 +222,10 @@ function(px4_add_board)
 		set(config_uavcan_num_ifaces ${UAVCAN_INTERFACES} CACHE INTERNAL "UAVCAN interfaces" FORCE)
 	endif()

+	if(UAVCAN_TIMER)


Suggested change

if(UAVCAN_TIMER)

if(UAVCAN_TIMER_OVERRIDE)

davids5 · 2020-08-14T09:39:48Z

src/drivers/uavcan/CMakeLists.txt

@@ -45,7 +45,9 @@ if(CONFIG_ARCH_CHIP)
 		set(UAVCAN_TIMER 1)
 	elseif(${CONFIG_ARCH_CHIP} MATCHES "stm32h7")
 		set(UAVCAN_DRIVER "stm32h7")
-		set(UAVCAN_TIMER 5) # The default timer is TIM5
+		if (DEFINED config_uavcan_timer_override)


Suggested change

if (DEFINED config_uavcan_timer_override)

set(UAVCAN_TIMER 5) # The default timer is TIM5

if (DEFINED config_uavcan_timer_override)

To check your changes. Please build all the H7 targets.
use find boards/ -name defconfig | xargs grep CONFIG_ARCH_CHIP_STM32H7=y to gee the list of what you have to build test.

I had changed it agian base on your suggestions ,could you please review it again.

Please see why the build fails. Run 'make cubepilot_cubeorange_default' and fix the issues.

davids5 · 2020-08-14T09:44:50Z

boards/cuav/x7pro/src/board_config.h

@@ -153,8 +153,11 @@
 /* High-resolution timer */
 #define HRT_TIMER               3  /* use timer3 for the HRT */
 #define HRT_TIMER_CHANNEL       3  /* use capture/compare channel 3 */
-#define STM32_RCC_APB1ENR  STM32_RCC_APB1LENR
-#define RCC_APB1ENR_TIM3EN RCC_APB1LENR_TIM3EN
+#define STM32_RCC_APB1ENR  	STM32_RCC_APB1LENR


How about the H7 micro_hal? platforms/nuttx/src/px4/stm/stm32h7/include/px4_arch/micro_hal.h

Then build all the H7 targets and fix them as well.

davids5

See comments.

davids5 · 2020-08-20T08:12:46Z

src/drivers/uavcan/CMakeLists.txt

@@ -45,6 +45,7 @@ if(CONFIG_ARCH_CHIP)
 		set(UAVCAN_TIMER 1)
 	elseif(${CONFIG_ARCH_CHIP} MATCHES "stm32h7")
 		set(UAVCAN_DRIVER "stm32h7")
+		set(UAVCAN_TIMER 5) # The default timer is TIM5
 		if (DEFINED config_uavcan_timer_override)
 		set (UAVCAN_TIMER  ${config_uavcan_timer_override})


Please indent this line. Also do you know How to squash the commits? Please squash the commits to the same files to one commit PER FILE. changed.

It is line 50
https://github.com/PX4/Firmware/blob/f222d046fccc2a75b49d8c37779ee459eac2cf86/src/drivers/uavcan/CMakeLists.txt#L50

davids5 · 2020-08-20T08:13:56Z

Ahh you also need to rebase this on master

gitfishup · 2020-08-21T08:05:55Z

@davids5 I had change it and fix the issue of "Run 'make cubepilot_cubeorange_default'".Could you please review it again? Thanks!

davids5 · 2020-08-21T08:46:43Z

@gitfishup the rebase looks incorrect. Please check out master and cherry-pick, in order just your commits,

If your remote for px4 is not called origin use that name instead.

git fetch origin
git checkout master
git checkout -b pr-can_x7pro-remix
git reset --hard origin/master
git cherry-pick e9d408b
git cherry-pick 5f85e5b
git cherry-pick 2413316
git cherry-pick d76167b

Then open a new PR with the Same title add Remix. In the description put X7Pro adds CAN driver #15348 continued here
then close this pr with a comment "Closed see <url of new PR>

… default.cmake

gitfishup · 2020-08-22T01:11:39Z

@davids5 How about this？

davids5

@gitfishup - Looking good - there are a few cosmetic changes.
@dagar any comments?

platforms/nuttx/src/px4/stm/stm32h7/include/px4_arch/micro_hal.h

… h7 boards

davids5

@CUAVcaijie @gitfishup - Thank you! This looks good. @dagar Please have a look and merge, when satisfied

thopiekar · 2020-08-28T07:14:30Z

There is one change requested by @dagar . Maybe he is waiting until this one is marked as solved? Because on the first look it seems that the work is not finished here 🤷‍♂️
Hope it gets merged soon 😄

dagar · 2020-08-28T16:15:41Z

Thanks everyone.

@CUAVcaijie

* X7Pro adds CAN driver * UAVCAN timer selection moved to default.cmake * Modify some details about @CUAVcaijie UAVCAN timer selection moved to default.cmake * Put some timer parameters to micro_hal.h from board_config.h. Fix all h7 boards Co-authored-by: honglang <honglang@cuav.net>

dagar reviewed Jul 17, 2020

View reviewed changes

dagar requested changes Jul 17, 2020

View reviewed changes

dagar added Admin: Enhancement (improvement) 💡 UAVCAN labels Jul 17, 2020

weekly-digest bot mentioned this pull request Jul 19, 2020

Weekly Digest (12 July, 2020 - 19 July, 2020) #15376

Closed

CUAVcaijie requested a review from dagar July 23, 2020 01:57

weekly-digest bot mentioned this pull request Jul 26, 2020

Weekly Digest (19 July, 2020 - 26 July, 2020) #15419

Closed

davids5 requested changes Aug 13, 2020

View reviewed changes

davids5 requested changes Aug 14, 2020

View reviewed changes

davids5 requested changes Aug 20, 2020

View reviewed changes

gitfishup force-pushed the can_x7pro branch from f222d04 to d76167b Compare August 21, 2020 07:54

CUAVcaijie and others added 3 commits August 21, 2020 17:30

X7Pro adds CAN driver

415ffbe

UAVCAN timer selection moved to default.cmake

fef7a76

Modify some details about @CUAVcaijie UAVCAN timer selection moved to…

393b4f7

… default.cmake

gitfishup force-pushed the can_x7pro branch from d76167b to f785ee8 Compare August 21, 2020 09:41

davids5 requested changes Aug 22, 2020

View reviewed changes

Put some timer parameters to micro_hal.h from board_config.h. Fix all…

52c924b

… h7 boards

gitfishup force-pushed the can_x7pro branch from f785ee8 to 52c924b Compare August 24, 2020 03:08

davids5 self-requested a review August 24, 2020 10:50

davids5 approved these changes Aug 24, 2020

View reviewed changes

gitfishup mentioned this pull request Aug 25, 2020

[WIP] cuav/nora: Enable UAVCAN #15608

Closed

dagar approved these changes Aug 28, 2020

View reviewed changes

dagar merged commit 536877c into PX4:master Aug 28, 2020

CUAVcaijie deleted the can_x7pro branch January 21, 2021 08:38

	set(config_uavcan_timer_num_ifaces ${UAVCAN_TIMER} CACHE INTERNAL "UAVCAN TIMER" FORCE)
	set(config_uavcan_timer_override ${UAVCAN_TIMER_OVERRIDE} CACHE INTERNAL "UAVCAN TIMER OVERRIDE FORCE)

	-DUAVCAN_${UAVCAN_DRIVER_UPPER}_TIMER_NUMBER=${config_uavcan_timer_num_ifaces}
	-DUAVCAN_${UAVCAN_DRIVER_UPPER}_TIMER_NUMBER=${UAVCAN_TIMER}

	if (DEFINED config_uavcan_timer_override)
	set(UAVCAN_TIMER 5) # The default timer is TIM5
	if (DEFINED config_uavcan_timer_override)

X7Pro adds CAN driver #15348

X7Pro adds CAN driver #15348

Conversation

CUAVcaijie commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar left a comment

Choose a reason for hiding this comment

ghulands commented Aug 10, 2020

CUAVcaijie commented Aug 10, 2020 • edited Loading

CUAVcaijie commented Aug 13, 2020

davids5 left a comment

Choose a reason for hiding this comment

davids5 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davids5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davids5 commented Aug 20, 2020

gitfishup commented Aug 21, 2020

davids5 commented Aug 21, 2020

gitfishup commented Aug 22, 2020

davids5 left a comment

Choose a reason for hiding this comment

davids5 left a comment

Choose a reason for hiding this comment

thopiekar commented Aug 28, 2020

dagar commented Aug 28, 2020

CUAVcaijie commented Aug 10, 2020 •

edited

Loading

davids5 Aug 13, 2020 •

edited

Loading