-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
[BACKPORT v1.13] Add Holybro Pixhawk Pi CM4 Baseboard Support #20182
Conversation
|
||
if ver hwtypecmp V5X00 V5X01 | ||
# V5X010001 ommited because not representable in this hardware format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is OK - we should ask @Igor-Misic @ThomasDebrunner what ID it is or to reserve it here in the PX4 V5X Spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a bad feeling about this, but I'm unsure. We are using V5X010001 (VER 0x10, REV 0x01), that's actually the current gen. Just because VER 0x10 is read from EEPROM (according to the ADC it would be ver 07x), it doesn't change the startup script, right? I believe this would break the build for Skynode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicovanduijn so this change is only for the release/v1.13
branch, and will go into v1.13.1. On main
this should all be fine. Does that resolve it?
My problem was that I don't know how V5X010001
would map back to the old V5Xabc
format.
From what I can see, the counting seems to be 0,1,2,3,...,9,a,10, which would be base 11 which is a bit unusual, but maybe I don't understand it 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's still conted as base16, we just skipped 0b,0c,0d,0e,0f and went to 0x10 after 0x0a to leave space for the remaining resistor space.
e.g. 0x00 - 0x0f : resistors
0x10 - 0xff: eeprom
at least that's my understanding of how it went.
But I can't tell you either how 0x010001 maps to the v5xabc format. I guess that just doesn't work, so we'll have to live with the fact that it won't boot on skynode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it, thanks! This is only v1.13, so I assume it doesn't really matter for you/Auterion anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's still conted as base16, we just skipped 0b,0c,0d,0e,0f and went to 0x10 after 0x0a to leave space for the remaining resistor space. e.g. 0x00 - 0x0f : resistors 0x10 - 0xff: eeprom
at least that's my understanding of how it went. But I can't tell you either how 0x010001 maps to the v5xabc format. I guess that just doesn't work, so we'll have to live with the fact that it won't boot on skynode
There are 11 code point that can be read from resistors.
0 - 0xb (0-11) are the values. 0x7 means read the EEPROM
In the EEPROM with current print format the using 3 digits each (see HW_INFO_REV_DIGITS and HW_INFO_VER_DIGITS and
the value are 0x10-0x3E7 (16-999)
Values are displayed and compared in HEX as lllvvvrrr
where lll is the label (V5, V6X DH etc) vvv is 3 hex digits for version and rrr is 3 hex digits for revision.
On X series the VER is the Base Board and the Rev is the FMUM.
In the code the #defines for the HW configs may use the abbreviated where thay fit.
#define V5X01 HW_VER_REV(0x0,0x1)
that is V5X0000001`
This was done to prevent coupling if we compiled with HW_INFO_xxx_DIGITS 2 or 3 or 5 as the value are still valid.
2c2704f
to
9c154e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self reviewed. I think this is correct.
We should do a quick check on all these new boards before tagging v1.13.1. |
Tested on Pixhawk 6x on CM4 baseboard:
3 gyros, 3 accel, 1 mag, 2 baros working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julianoes - see inline comments
|
||
if ver hwtypecmp V5X00 V5X01 | ||
# V5X010001 ommited because not representable in this hardware format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is OK - we should ask @Igor-Misic @ThomasDebrunner what ID it is or to reserve it here in the PX4 V5X Spec
// Base FMUM | ||
#define V5X00 HW_VER_REV(0x0,0x0) // FMUV5X, Rev 0 | ||
#define V5X10 HW_VER_REV(0x1,0x0) // NO PX4IO, Rev 0 | ||
#define V5X01 HW_VER_REV(0x0,0x1) // FMUV5X I2C2 BMP388, Rev 1 | ||
#define V5X02 HW_VER_REV(0x0,0x2) // FMUV5X, Rev 2 | ||
#define V5X40 HW_VER_REV(0x4,0x0) // FMUV5X, HB CM4 base Rev 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be dropped. @nicovanduijn Please correct me but the FMUM REV 0 was not shipped other then in prototypes. Once the BMP 388 was moved to i2c2 the FMUM REV e was bumped to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ that's incorrect, a batch of 40 FMUM REV0 was definitely shipped to early customers. I don't know how many of them are still operational
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if this is about being able to support those FMUs (RC13) on the holybro CM4 base board, we can probably ignore it. We shipped out full skynode assemblies and not single FMUs and I believe it to be safe to assume that those will not be mixed with other base boards
//#define V6X50 HW_VER_REV(0x5,0x0) // FMUV6X, HB Mini Rev 0 // never shipped | ||
//#define V6X51 HW_VER_REV(0x5,0x1) // FMUV6X, BMI388 I2C2 HB Mini Rev 1 // never shipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for V6X these have to stay for a bit longer
boards/px4/fmu-v6x/src/manifest.c
Outdated
//{V6X40, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // HB CM4 base // never shipped | ||
//{V6X41, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // BMP388 moved to I2C2 HB CM4 base // never shipped | ||
{V6X43, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // BMP388 moved to I2C2, HB CM4 base Sensor Set 3 | ||
{V6X44, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // BMP388 moved to I2C2, HB CM4 base Sensor Set 4 | ||
//{V6X50, hw_mft_list_v0650, arraySize(hw_mft_list_v0650)}, // HB Mini // never shipped | ||
//{V6X51, hw_mft_list_v0650, arraySize(hw_mft_list_v0650)}, // BMP388 moved to I2C2 HB Mini // never shipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//{V6X40, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // HB CM4 base // never shipped | |
//{V6X41, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // BMP388 moved to I2C2 HB CM4 base // never shipped | |
{V6X43, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // BMP388 moved to I2C2, HB CM4 base Sensor Set 3 | |
{V6X44, hw_mft_list_v0640, arraySize(hw_mft_list_v0640)}, // BMP388 moved to I2C2, HB CM4 base Sensor Set 4 | |
//{V6X50, hw_mft_list_v0650, arraySize(hw_mft_list_v0650)}, // HB Mini // never shipped | |
//{V6X51, hw_mft_list_v0650, arraySize(hw_mft_list_v0650)}, // BMP388 moved to I2C2 HB Mini // never shipped | |
{V6X40, hw_mft_list_v0600, arraySize(hw_mft_list_v0600)}, // HB CM4 base RC01 FMUM | |
{V6X41, hw_mft_list_v0600, arraySize(hw_mft_list_v0600)}, // BMP388 moved to I2C2 HB CM4 base RC01 FMUM | |
{V6X43, hw_mft_list_v0600, arraySize(hw_mft_list_v0600)}, // BMP388 moved to I2C2, HB CM4 base Sensor Set 3 | |
{V6X44, hw_mft_list_v0640, arraySize(hw_mft_list_v0600)}, // BMP388 moved to I2C2, HB CM4 base Sensor Set 4 | |
{V6X50, hw_mft_list_v0650, arraySize(hw_mft_list_v0650)}, // HB Mini RC01 FMUM | |
{V6X51, hw_mft_list_v0650, arraySize(hw_mft_list_v0650)}, // BMP388 moved to I2C2 HB Mini RC01 FMUM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two changes combined seem to only increase flash by 40 bytes or so, so that should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davids5 this suggests to add V6X51 back in but it's not listed in the initSPI below. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicovanduijn do have any revs greater then RC01 of the V6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet, but we might in the near future. Is there a community table for the rev and vers for v6x? We should probably also reserve one resistor each to denote " read from EEPROM ". On v5x we have that for ver, but not yet for rev if I'm not mistaken
@@ -84,9 +84,9 @@ constexpr px4_spi_bus_all_hw_t px4_spi_buses_all_hw[BOARD_NUM_SPI_CFG_HW_VERSION | |||
}), | |||
}), | |||
|
|||
initSPIHWVersion(V6X50, { | |||
initSPIHWVersion(V6X43, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add V6X43 and keep V6X50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's plus around 700 B of flash, unfortunately.
And note, this PR is just the backport to v1.13.
9c154e1
to
4ca31bb
Compare
@davids5 thanks for the review. I have followed your suggestions and added two commits. Note that this is the backport to v1.13, so I have to forwardport 😄 them back to main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this looks good.
@vincentpoont2 have you tested the bases and all the FMUM with all the IMUs versions?
@julianoes tested on V6 RC01 HW
|
Is there a shared table for the ver and rev for v6x somewhere? For v5x I also struggle to find that every time, I feel like this should be somehow in a more visible place. I started maintaining my own for v5x... (which is dumb, I have to admit) REV = FMUM ------------- v6x ------------------ |
@nicovanduijn should we have some shared Google sheet for that? |
I think it's on these sheet at the moment, but it's not very visible and updated. https://docs.google.com/spreadsheets/d/192yS1ewajvsgp90g_rVM3QiBnM678sPVkYVWaQhRWBQ/edit#gid=306908149 |
It is the HW REV and VER ID tab on the pinouts. V5X: @mrpollo - Would you please share the link to the 5x and 6X pinout with comment access |
What's the overall state here? This is a lot of discussion for a simple cherry-picked backport. |
4ca31bb
to
a60aa40
Compare
@dagar @julianoes - forward port is merged, let's get this in followed by a rebase of #19819 |
a60aa40
to
83fe707
Compare
Thanks @davids5, I'm just checking CI again. |
fa116cb
to
24499cf
Compare
add V5X004002
My assumption is that the bus are numbered < 127. This saves about 100 bytes of flash.
As I understand it, only Rev 1 and Rev 2 were shipped by HB, and likely to be used for the Mini and CM4.
As I understand it, only Rev 3 and Rev 4 were shipped by HB for Mini and CM4, and are likely to be used for it. It would be nice to have all combinations but it requires quite some flash in the current implementation.
- Removed commented out config data. - hw_mft_list_v0540 was the same as hw_mft_list_v0500
- alias from hw_mft_list_v0650 to hw_mft_list_v0600 as it is the same - add V6X50 again
24499cf
to
7c846dc
Compare
This fixes the errors showing up at startup for me with a Black Cube.
This was forgotton with all the merges and shuffling previously.
@julianoes is this ready to come in now or was there any more you wanted to add? |
@davids5 ok thanks. @vincentpoont2 just tested this on V5X mini as well which was missing until I added the last commit. I'm merging this now, and then forward porting the commits required for main. |
Backport of #20086.