Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Brushless electric motor example #573

Closed
wants to merge 31 commits into from

Conversation

pbecchi
Copy link
Contributor

@pbecchi pbecchi commented Jan 21, 2022

This is DJI F450 quadcopter example to test behaviour of new brushless DC electric motor.

pbecchi and others added 30 commits December 20, 2021 00:06
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
@seanmcleod
Copy link
Member

@pbecchi I'm not sure how you're generating the pull request overall. When I took an initial look at the list of commits it lists 31 commits, I'm pretty sure the 31 commits weren't all relevant to simply adding the new quadcopter model. Also when I take a look at the files changed in addition to the files added for the quadcopter I noticed FGBrushlessDCMotor.cpp was listed, with the diff showing changes that have already been merged into JSBSim:master.

Just generally makes things more confusing to review.

Compare it to this pull request - #559 where there was a single commit on Jan 6 when the pull request was created and then after feedback from @bcoconni on the pull request I pushed another commit on Jan 7 to this branch/pull request. So only the relevant commits show up in the pull request, and not 31 commits (https://github.com/JSBSim-Team/jsbsim/pull/573/commits) and also only the relevant files.

My workflow when generating a pull request like the example above is to first merge the latest JSBSim:master into my fork's master. Then create a new branch, e.g. F450QuadModel and only push commits to this new branch that are specific to this new feature/bug fix. Then push this new branch of mine to my fork and generate a pull request from that to JSBSim:master.

Once the pull request is merged into JSBSim:master then you can optionally delete the branch, e.g. F450QuadModel in your fork. Also at this point pull and merge in the latest JSBSim:master into your fork's master. Creating a new branch for any further new features/bug fixes, i.e. repeating the process.

When we merge in the pull request we perform a Squash and merge which will squash the multiple commits if any of this dedicated feature/bug fix branch.

@pbecchi
Copy link
Contributor Author

pbecchi commented Jan 21, 2022

This PR should consist on only one commit: c137c20
I have seen that most of the old commits are also showing up and I dont know why.
Anyhow i will do has you suggest,:

  1. I will cancel this PR,
  2. I will create a new branch with this example and
  3. I will try again to make again the PR ....

@pbecchi pbecchi closed this Jan 21, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants