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.

Sign up for GitHub

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

Jump to bottom

Rover Sailboat: refactor the mainsail/wingmast/mast_rotation logic #26152

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Rover Sailboat: refactor the mainsail/wingmast/mast_rotation logic #26152

merged 3 commits into from
Feb 21, 2024

Conversation

kwikius
Copy link
Contributor

@kwikius kwikius commented Feb 6, 2024

Rover Sailboat: refactor the mainsail/wingmast/mast_rotation logic and make non const sailboat functions private to the sailboat class. Refactor saves around 128 bytes of text image size (in SITL anyway) and easier to read the source code

IamPete1
IamPete1 reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Nice improvement! Looks like there should not be any change in behavior?

Rover/sailboat.cpp Outdated
mast_rotation_out = 0.0f;
return;
relax_sails();
}else{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}else{
} else {

Rover/sailboat.cpp Outdated
//

// mainsail_out represents a range from 0 to 100
float mainsail_out = 100.f;
Copy link
Member

@IamPete1 IamPete1 Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I find the . harder to read than .0, we don't really need the f either. (I know there all over already)

Rover/sailboat.cpp Outdated

if (motor_state == UseMotor::USE_MOTOR_ALWAYS) {
relax_sails();
}else{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}else{
} else {

@IamPete1 IamPete1 self-assigned this Feb 6, 2024
@kwikius
Copy link
Contributor Author

kwikius commented Feb 7, 2024 •
edited
Loading

Nice improvement! Looks like there should not be any change in behavior?

There is a change in behaviour from https://github.com/ArduPilot/ardupilot/blob/master/Rover/mode_manual.cpp#L38 , which I think was a bug? So I changed behaviour to as if g2.motors.set_mast_rotation(desired_mast_rotation);

NOTE In practise this bug had no effect so no actual change in behaviour since both desired_mast_rotation and desired_wingsail have the same value here currently, after call to get_pilot_desired_mainsail(...)

kwikius added a commit to kwikius/ardupilot that referenced this pull request Feb 7, 2024
@kwikius
Rover Sailbaot: Tidy up ArduPilot#26152
83375be
@kwikius
Copy link
Contributor Author

kwikius commented Feb 7, 2024 •
edited
Loading

EDIT: Fixed I hope.
(messed up the PR. Shouldn't have incorporated latest changes from master )

kwikius added a commit to kwikius/ardupilot that referenced this pull request Feb 7, 2024
@kwikius
Rover Sailboat: reset and Tidy up ArduPilot#26152
1e6d6e7
@kwikius kwikius force-pushed the kwikius_sailboat_sails_refactor branch from 83375be to 1e6d6e7 Compare February 7, 2024 20:50
@kwikius
Copy link
Contributor Author

kwikius commented Feb 7, 2024

Reset to head at original PR prior to rebase

kwikius added a commit to kwikius/ardupilot that referenced this pull request Feb 8, 2024
@kwikius
Rover Sailboat: reset and Tidy up ArduPilot#26152
e3feab2
@kwikius kwikius force-pushed the kwikius_sailboat_sails_refactor branch from 1e6d6e7 to e3feab2 Compare February 8, 2024 07:41
kwikius added a commit to kwikius/ardupilot that referenced this pull request Feb 18, 2024
@kwikius
Rover Sailboat: reset and Tidy up ArduPilot#26152
4618e17
@kwikius kwikius force-pushed the kwikius_sailboat_sails_refactor branch from e3feab2 to 4618e17 Compare February 18, 2024 09:18
IamPete1
IamPete1 reviewed Feb 18, 2024
View reviewed changes
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Looking good, just that friend stuff and I think we can merge.

Rover/sailboat.h Outdated
Comment on lines 22 to 28
friend class Rover;
friend class Mode;
friend class ModeManual;
friend class ModeHold;
friend class ModeLoiter;
friend class ModeAcro;
friend class RC_Channel_Rover;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are needed for this PR? There are no changes to most of those classes.

Copy link
Contributor Author

@kwikius kwikius Feb 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I modified Sailboat class member function access to make all the non const Sailboat member functions private and so those classes calling private Sailboat functions ( ie Sailboat state modifying functions) require friend access to Sailboat. The overall idea is limit who can change the state as much as possible. If non const member functions are public, anyone can change state thus making debugging harder. The precedent is similar use of friends in Rover class https://github.com/ArduPilot/ardupilot/blob/master/Rover/Rover.h#L77

Shall I revert it to those non const member functions of Sailboat being public?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. I had not appreciated the change in scope of those functions. I think my preference would be to keep the current structure.

They key difference is that all of the params and internal state variables are currently private. So cannot be changed externally. It we add friends then they can all reach in any change things they probably shouldn't. EG you should call set_motor_state not just change motor_state. That is currently enforced.

If there are things that should defiantly not be able to change anything (those class which you did not have to add as friends) then we could go via a const reference to the class. But i'm not really sure there are any that would need this?

@kwikius
Rover Sailboat: refactor the mainsail/wingmast/mast_rotation logic an…
c9dae4a 
…d make non const sailboat functions private to the sailboat class.

Saves around 128 bytes of text image size (in SITL anyway)
@kwikius
Rover Sailboat: reset and Tidy up ArduPilot#26152
66fa602
kwikius added a commit to kwikius/ardupilot that referenced this pull request Feb 20, 2024
@kwikius
revert friends and private Sailboat functions. See ArduPilot#26152 (c…
dba1f19 
…omment)
@kwikius kwikius force-pushed the kwikius_sailboat_sails_refactor branch from 4618e17 to dba1f19 Compare February 20, 2024 19:45
@IamPete1 IamPete1 force-pushed the kwikius_sailboat_sails_refactor branch from dba1f19 to 2e9f0c5 Compare February 20, 2024 23:46
IamPete1
IamPete1 approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Looks good, Thanks, I have just added Rover: Sailboat to start of the last commit message.

@tridge tridge requested a review from rmackay9 February 21, 2024 07:34
@tridge tridge removed the DevCallEU label Feb 21, 2024
@rmackay9 rmackay9 merged commit d867364 into ArduPilot:master Feb 21, 2024
58 checks passed
rmackay9 pushed a commit that referenced this pull request Feb 21, 2024
@kwikius @rmackay9
Rover Sailboat: reset and Tidy up #26152
057d7dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IamPete1 IamPete1 IamPete1 approved these changes

@rmackay9 rmackay9 rmackay9 approved these changes

Assignees

@IamPete1 IamPete1

Labels
Sailboat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kwikius @rmackay9 @IamPete1 @tridge