Skip to content

bugfix for marlin#822

Closed
MangaValk wants to merge 1 commit intostm32duino:masterfrom
MangaValk:marlin_fix
Closed

bugfix for marlin#822
MangaValk wants to merge 1 commit intostm32duino:masterfrom
MangaValk:marlin_fix

Conversation

@MangaValk
Copy link

Summary

This PR fixes an issue i had on Marlin when using multiple servos.
Issue: MarlinFirmware/Marlin#16138

@fpistm
Copy link
Member

fpistm commented Dec 9, 2019

Hi @MangaValk

In the official Arduino Servo library:

could you be more precise about why you need to do those updates?
And add this explanation in the commit message.
Thanks

@fpistm fpistm added the waiting feedback Further information is required label Dec 9, 2019
@fpistm fpistm requested a review from ABOSTM December 9, 2019 07:07
int readMicroseconds(); // returns current pulse width in microseconds for this servo (was read_us() in first release)
bool attached(); // return true if this servo is attached, otherwise false
private:
protected:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a reason to change this attribute to protected instead of private.
This variable should not be modified outside of the class.
I can understand that you need to read this variable, but in this case we can rather implement an accessor.

Copy link
Author

Choose a reason for hiding this comment

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

A getter function would be fine too. I'm not a professional c++ programmer, so I thought a protected var would be fine.

static HardwareTimer TimerServo(TIMER_SERVO);

uint8_t ServoCount = 0; // the total number of attached servos
static uint8_t ServoCount = 0; // the total number of attached servos
Copy link
Contributor

Choose a reason for hiding this comment

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

From my point of view there is no restriction to define this variable static.
Nevertheless, if this cause you issue in Marlin, this probably means you have a variable with the same name ? Can you confirm ? maybe you also need to take some action on marlin to avoid conflict too?

Copy link
Author

Choose a reason for hiding this comment

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

Here is link to the marlin change I made MarlinFirmware/Marlin#16151 (review)

Somehow this made it work, if I turn it back to normal it stops working.

@fpistm
Copy link
Member

fpistm commented Dec 9, 2019

@MangaValk
what is strange is for other arch supported by Marlin and using the same Servo Class definition (Official Arduino), this is not requested.
My guess is there is an issue how it is implemented for the HAL_STM32 in Marlin.
@ABOSTM already tested the library with multiple Servo without any issue.

@MangaValk
Copy link
Author

I'm closing this PR since someone found a fix that works without these changes.

@MangaValk MangaValk closed this Dec 9, 2019
@MangaValk MangaValk deleted the marlin_fix branch December 10, 2019 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting feedback Further information is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments