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 ?
Thank you again for contributing to Orekit.
Contributing License Agreement is available here: https://www.orekit.org/doc/orekit_governance_v4.pdf
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 . 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
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 .
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,
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.
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
*ThrustDirectionVector moved to
In progress : checkstyle formatting, test and comments on ThrustDirectionAndAttitudeProvider
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.
I will look at your implementation this afternoon.
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:
- ThrustDirectionAndAttitudeProvider constructor
- ConstantThrustDirectionVector constructor
- EventBasedManeuverTriggers constructor
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
Except these remarks, everything looks good. Classes implementation and tests coverage look good.
Thank you again.
Thank you for the review. I renamed
ThrustDirectionProvider, fixed the test and add the missing javadoc