Low thrust maneuver contribution

Hi,
Following the discussion on maneuver packages we want to propose a contribution on implementing low thrust maneuvers.
We propose 2 main improvements :

  • use of start/stop detectors different than date detectors to have peridoic thrust periods
  • an attitude super class that can provide the spacecraft attitude from the required thrust direction

Our contribution would be based on the new architecture. on the branch maneuver-package.
I will provide more technical detail once working on it.

I think there is a contribution licence agreement to sign, could you giveme the link ?

Hi @Mikael

Thank you again for contributing to Orekit.

Contributing License Agreement is available here: https://www.orekit.org/doc/orekit_governance_v4.pdf
(Annex section)

Regards,
Bryan

Thank you @Mikael.

That is a great idea.

Maxime

Hi,
The contribution is ready for discussion : the initial commit is here https://gitlab.orekit.org/mikael/orekit/-/commit/f3bf486e9e608562b6ea4af6993c823270d40a84

Feel free to propose new names for the classes and to make comments on the code/architecture. We may need to add more units tests.

Thank you Mikael!
We will try to review your contribution ASAP.
I personally don’t have much time to spare but I’ll do my best.

Hi again @Mikael,

I just checked out your branch. I hadn’t much time to review the code but I have a few preliminary comments:

  • Checkstyle is not activated :wink: . The Orekit checkstyle rules’ file is at the root of the git repository, in checkstyle.xml. Tell me if you need help to configure your Eclipse IDE with it.

  • Do you think the DateBasedManeuverTriggers class could be an extension of your EventBaseManeuverTriggers?

  • ThrustDirectionProvider: Since it is an AttitudeProvider I would (personally) put it in the attitude package. If I understand well the purpose here is to give a satellite attitude given a desired thrust and an underlying attitude provider. May I suggest that you add some comments to describe what is done here and the purpose because, without digging into the code, it is a little hard to tell :slight_smile:.
    Regarding the name, maybe something like Maneuver(Aligned|Pointing|Direction)Provider would be more understandable, I don’t know.

  • (Constant&Variable)ThrustDirectionVector: I’m wondering if we shouldn’t put these 2 in the propulsion sub-package. This would keep the maneuvers package “cleaner”, with only high-level maneuvers’ models, and the building blocks of the maneuvers will be dispatched in the sub-packages.
    Also I’m wondering if the ThrustDirectionVector models couldn’t be used inside the PropulsionModels but I’ll need more time to think about it.

  • Copyrights: I’m not an expert in copyright headers but sometimes you used the “CS Group” copyright which doesn’t seem right since you are contributing in the name of your company. And sometimes just “Copyright Exotrail 2019”, without stating that it is contributed to the public domain. I think you should build your own copyright header. You can base it on what is done for the CS one. I wouldn’t know if there are some mandatory keywords needed there. In my opinion a copy/paste of CS one, replacing CS with Exotrail © would be fine.

Well, this was quite long after all.
Thanks again for this,
Maxime

The template defined in the file license-header.txt must be used. In this, you can set the copyright holder (i.e. Exotrail) in the first line, but the remainder of the header must be used as is. It is checked by checkstyle rules. Note that typically the line either specifies a copyright holder (for example for HolmesFeatherstoneAttractionModel.java it is CS GROUP and for OEMWriter.java is is Applied Defense Solutions) or specifies there are no copyright holder and the code is in the public domain (for example Relativity.java). From a legal point of view, you cannot contribute it as public domain since you are in France. Public domain in France happens only 70 years after the death of the author, or for very special things like court rulings if I remember well. The header with public domain is mainly used for files created by contributors who are agents from a US government agency as the legal rules that apply there are quite different from what applies in France.

This header is mandatory as it clearly states the complete library can be published under the terms of the Apache License.

Thank you for the feedback. I pushed a new commit :

  • headers updated
  • added unit tests and removed backward propagation
  • ThrustDirectionProvider renamed to ThrustDirectionAndAttitudeProvider and moved to propulsion sub-package. It is highly linked to maneuvers so I prefer keeping it in the maneuver package. But I understand that it can be better to group all attitude provider together, we will move it there if needed. We will add more comments soon.
  • about DateBasedManeuverTriggers which coud be an extension of EventBaseManeuverTriggers, I don’t think that is a good idea for the moment. This class does not manage the backward propagation and the FieldEvents
  • *ThrustDirectionVector moved to propulsion sub-package

In progress : checkstyle formatting, test and comments on ThrustDirectionAndAttitudeProvider

Thanks Mikael.
I understand your point on the attitude provider. Let’s keep it there for now.

I pushed a new version with new tests and more comments.
It is ready for final review.

Hi Mikael,

I will look at your implementation this afternoon.

Bryan

I just checked out your branch. First, thank you again for the contribution.

Here some comments:

  • There is a failure for testMessageNumber test in OrekitMessagesTest. As you added 3 new messages, you have to set the reference number of messages at 208 instead of 205.

  • There are are some JavaDoc absences:

    • ConfigurableLowThrustManeuver.buildBasicConstantThrustPropulsionModel(…)
    • ConfigurableLowThrustManeuver.getThrustDirectionProvider(…)
    • ThrustDirectionAndAttitudeProvider constructor
    • ThrustDirectionAndAttitudeProvider.checkParameterNotNull(…)
    • ThrustDirectionAndAttitudeProvider.getThrusterAxisInSatelliteFrame(…)
    • ThrustDirectionAndAttitudeProvider.getAttitudeFromFrame(…)
    • ThrustDirectionAndAttitudeProvider.getManeuverAttitudeProvider(…)
    • ConstantThrustDirectionVector constructor
    • EventBasedManeuverTriggers constructor
    • EventBasedManeuverTriggers.getStartFiringDetector(…)
    • EventBasedManeuverTriggers.getStopFiringDetector(…)
    • EventBasedManeuverTriggers.setFiring(…)
    • EventBasedManeuverTriggers.getTriggeredEnd(…)
    • EventBasedManeuverTriggers.getTriggeredStart(…)
  • This is just my opinion but I’m a bit surprised to see ConstantThrustDirectionVector class implementing VariableThrustDirectionVector interface. I’m wondering if the name of VariableThrustDirectionVector interface can change to ThrustDirectionVector.

Except these remarks, everything looks good. Classes implementation and tests coverage look good.

Thank you again.
Bryan

Thank you for the review. I renamed VariableThrustDirectionVector to ThrustDirectionProvider, fixed the test and add the missing javadoc