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.

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

AP_HAL_ChibiOS: add hwdef files for NarinFC-H7 #29241

Merged
merged 2 commits into from
Apr 8, 2025

Conversation

vololand
Copy link
Contributor

@vololand vololand commented Feb 6, 2025

AP_HAL_ChibiOS: add hwdef files for NarinFC-H7
(ArduPilot/ardupilot:master <--- vololand/ardupilot:NarinFC-H7)

@vololand
Copy link
Contributor Author

vololand commented Feb 12, 2025

(ardupilot_wiki) wiki PR #6611 :
Added NarinFC-H7 products documentation #6611

@vololand
Copy link
Contributor Author

Someone please tell me what to do.

@vololand
Copy link
Contributor Author

#Peterbarker
Please help me, how can I complete PR?

Copy link
Contributor Author

@vololand vololand left a comment

Choose a reason for hiding this comment

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

AP_HAL_ChibiOS: add hwdef files for NarinFC-H7

@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Feb 26, 2025

#Peterbarker Please help me, how can I complete PR?
Sorry just saw this as I was checking newbd PR labels

  1. request reviews from me and AndyPiper (I just did...will look at it this week)
  2. make changes as required and verify that you have tested (see wiki on suggested tests)
  3. we then can mark it for DevTeam review and merge

@Hwurzburg
Copy link
Collaborator

@vololand why do you add merge commits to this pr and the wiki one? it clutters the commit history and invalidates any review comments...

Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

good start on readme but needs more info....see this draft guide I have done:
ArduPilot/ardupilot_wiki#6645

@vololand
Copy link
Contributor Author

vololand commented Mar 4, 2025

@vololand why do you add merge commits to this pr and the wiki one? it clutters the commit history and invalidates any review comments...

@Hwurzburg
Sorry, I don't understand what you mean.
Can you tell me what to do?

@vololand vololand requested a review from Hwurzburg March 4, 2025 09:39
@vololand
Copy link
Contributor Author

vololand commented Mar 5, 2025

@vololand why do you add merge commits to this pr and the wiki one? it clutters the commit history and invalidates any review comments...

@Hwurzburg
Is your question in the example below?

example 1:
image

example 2:
image

example 3:
image

@Hwurzburg
Copy link
Collaborator

yes you dont merge master into your working branch, you REBASE it on master if you want to update it....but updating to master will be done when it get approved and merged into master, so you don't have to continually update your PR unless something master impacts it (not usual for bd PRs)

@vololand
Copy link
Contributor Author

vololand commented Mar 6, 2025

yes you dont merge master into your working branch, you REBASE it on master if you want to update it....but updating to master will be done when it get approved and merged into master, so you don't have to continually update your PR unless something master impacts it (not usual for bd PRs)

@Hwurzburg
Thank you for the explanation.
I won't update it until PR is completed.

Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

  1. Serial6/UART7 connector not show or pins listed
  2. If we want to recommend it for duplex RC like CRSF it MUST have DMA...remove the NODMA in hwdef...it allocates okay, I checked
  3. you say all motor/servco outputs are Bidir DShot capable...at this time NONE are and outputs 13 and 14 cant do DSHot, which you state they can
  4. you define a spare ADC ports, but they are not show in pinout image or lists
  5. the APJ_BOARD_ID in both hwdef files should be AP_HW_NarinH7 not 1183
  6. the GPIO pin numbers for RSSI_IN, the spare ADCs should be listed in the readme

Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

  1. you added ADC and RSSI conenctor but images do not show where they are located
  2. SERAIL6 need DMA capable notation in readme
  3. You have batt mon params in defaults.parm file (those should be moved to hwdef defines not in defaults file), but if you ship the autopilot with the DroneCAN power module, this should be what the Battery Monitor section in the Readme discusses not analog monitor....if you ship an analog monitor then BATT_MONITOR should be 4 in the hwdef....if you will ship with either option, then both should be discussed in the readme section instructions on how to configure for analog or DroneCAN.

getting close

@Hwurzburg
Copy link
Collaborator

Thanks...I have tweaked the README a little, squashed, and split...I have approved and marked for next devcall

@Hwurzburg Hwurzburg self-requested a review March 25, 2025 14:03
@Hwurzburg Hwurzburg added DevCallTopic WikiNeeded needs wiki update labels Mar 25, 2025
@peterbarker
Copy link
Contributor

Ping @andyp1per

@tridge
Copy link
Contributor

tridge commented Apr 2, 2025

@andyp1per will review

@andyp1per
Copy link
Collaborator

@vololand I added bdshot to PWM 1-8 - this works quite nicely. Do you really intend to ship all of these IMUs? If not you should remove the ones that will never be on the board as they are currently chewing up DMA channels for SPI.

@Hwurzburg Hwurzburg self-requested a review April 2, 2025 11:48
@Hwurzburg
Copy link
Collaborator

Readme needs changes after Andy's commits...in output and RCin sections

@vololand
Copy link
Contributor Author

vololand commented Apr 3, 2025

@vololand I added bdshot to PWM 1-8 - this works quite nicely. Do you really intend to ship all of these IMUs? If not you should remove the ones that will never be on the board as they are currently chewing up DMA channels for SPI.

@andyp1per
If it becomes a problem in the future, I will change it then.
Thank you.

@vololand vololand requested a review from andyp1per April 3, 2025 07:03
@vololand
Copy link
Contributor Author

vololand commented Apr 3, 2025

Readme needs changes after Andy's commits...in output and RCin sections

@Hwurzburg
If you deliver what you want to modify, I will apply it.
Thank you.

@vololand vololand requested a review from Hwurzburg April 4, 2025 00:13
@Hwurzburg Hwurzburg removed the request for review from andyp1per April 4, 2025 17:12
@tridge tridge merged commit a9077ae into ArduPilot:master Apr 8, 2025
40 checks passed
@vololand
Copy link
Contributor Author

vololand commented Apr 8, 2025

@Hwurzburg
Thank you for your help.
When will NarinFC-H7 be uploaded to the firmware server ("https://firmware.ardupilot.org/Copter/stable-4.5.7/ "?
When will the NarinFC-H7 firmware update be available in GCS?

@Hwurzburg
Copy link
Collaborator

  1. this is probably never going to be backported to 4.5...that ship sailed long ago
  2. I will mark for backport to 4.6 beta 6, but there is no guarantee since its immenient so it might only be in the beta for 4.7
  3. it will take a few days to appear in "latest" on the firmware.ardupilot.org server...then it can be downloaded by GCS using "show all build options" , or similar,on their firmware install pages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants