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

SKYSTARSH743HD remove non-existent S9 & S10. Add servos target #9214

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

sensei-hacker
Copy link
Collaborator

Adds an option to use MC servos on S5-S8, and removes definitions for S9 & S10 which do not seem to be physically present.

This target was originally written with 8 PWM outputs, as specified by the manufacturer.
A past commit that otherwise just cleaned up whitespace in targets added S9 & S10. This may have been in error as S9 & S10 do not exist as far as we can determine.

@@ -36,15 +36,19 @@ timerHardware_t timerHardware[] = {
DEF_TIM(TIM4, CH1, PD12, TIM_USE_MC_MOTOR | TIM_USE_FW_SERVO, 0, 2), // S3
DEF_TIM(TIM4, CH2, PD13, TIM_USE_MC_MOTOR | TIM_USE_FW_SERVO, 0, 3), // S4

DEF_TIM(TIM5, CH1, PA0, TIM_USE_MC_MOTOR | TIM_USE_FW_SERVO, 0, 4), // S5
#if defined(SKYSTARSH743HD_SERVOS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see some targets with both TIM_USE_MC_SERVO and TIM_USE_MC_MOTOR on the same pin.

Wouldn't that be better than a different target for servos?

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 hadn't noticed that before, but I see that is true.

Looking at the code, it appears that both flags can be used so long as the number of required motors are assigned to lower-numbered PWM outputs.
That is, it wouldn't work to have both MC_MOTOR and MC_SERVO on S1 if you have any motors. But it should work in this case. Thanks.

I'll update the 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.

@mmosca I see that in many such cases, such as the Matek boards, the pins that are declared MC_MOTOR | MC_SERVO have their own timers. Therefore there cannot be a conflict with other PWM on the same timer.

As discussed on Discord, testing shows that CAN cause / allow a conflict in cases like this, where there is more than one output for that timer.

I am updating the target to use the approach @DzikuVx suggested.

@@ -36,15 +36,19 @@ timerHardware_t timerHardware[] = {
DEF_TIM(TIM4, CH1, PD12, TIM_USE_MC_MOTOR | TIM_USE_FW_SERVO, 0, 2), // S3
DEF_TIM(TIM4, CH2, PD13, TIM_USE_MC_MOTOR | TIM_USE_FW_SERVO, 0, 3), // S4

DEF_TIM(TIM5, CH1, PA0, TIM_USE_MC_MOTOR | TIM_USE_FW_SERVO, 0, 4), // S5
#if defined(SKYSTARSH743HD_SERVOS)
Copy link
Member

Choose a reason for hiding this comment

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

There is an alternative solution. Set S5-S8 as MC_SERVO and then use of flag output_mode to MOTORS enables the X8 support. But we indeed do not need 2 targets for this board

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 have updated the branch according to your comment.

@DzikuVx DzikuVx added the Release Notes Add this when a PR needs to be mentioned in the release notes label Aug 25, 2023
@DzikuVx DzikuVx added this to the 7.0 milestone Aug 25, 2023
@DzikuVx DzikuVx merged commit 029ae30 into iNavFlight:master Aug 25, 2023
14 checks passed
@mmosca mmosca mentioned this pull request Aug 30, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release Notes Add this when a PR needs to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants