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 ?
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 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 .
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.
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
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.
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.