Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a --image-time flag to the distance command that records when an image was captured, enabling more accurate satellite position calculations by using the actual image capture time instead of the current system time. The timestamp is stored in the output .found file.
- Adds new
strtodatetimeconverter function to parse datetime strings in YYYY-MM-DD HH:MM:SS format - Replaces automatic timestamp generation with user-provided image capture time in position records
- Updates all distance command tests and README documentation to include the new time parameter
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/providers/converters.hpp | Adds strtodatetime function to convert string timestamps to DateTime structures |
| src/command-line/parsing/options.hpp | Adds image-time option definition for the distance command |
| src/command-line/execution/executors.cpp | Modifies OutputResults to use provided imageTime instead of calling getUT1Time() |
| test/integration/integration-test.cpp | Updates integration tests to include the new --image-time parameter and removes obsolete test |
| test/command-line/parsing/parser-test.cpp | Adds parser tests for the new imageTime field |
| test/command-line/execution/executors-test.cpp | Updates executor test to include sample imageTime data |
| README.md | Updates documentation and examples to show usage of the new --image-time flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nguy8tri
left a comment
There was a problem hiding this comment.
This is by no means comprehensive, but great job, just left some comments!
nguy8tri
left a comment
There was a problem hiding this comment.
Good job on the fixes. This time, my comments are more towards having you think critically about some code changes you've made. Please respond to my comments as appropriate. Also if it takes a long time for me to review your code, ping me on slack so I can make time for it.
| * | ||
| * All coordinates are in meters. | ||
| */ | ||
| typedef Vec3 ECEFCoordinates; |
There was a problem hiding this comment.
At this point, it probably belongs in style.hpp instead (or possibly the output file, but you should defend either decision you make). Alternately, you could forgo ECEFCoordinates altogether and just use Vec3. The primary reason I use typedefs is to clarify what kind of info is being fed between and as an output from each pipeline, but this is not one of those data types. You could still keep the typedef, but also defend that decision as well.
There was a problem hiding this comment.
Since Quaternion is also defined in attitude-utils.hpp, wouldn't it be appropriate to define the different coordinate systems in the same file? I do think that it may be worth separating the mathematical definitions like Vec2 and Vec3 from definitions related to the coordinate system, like Attitude, Quaternion, etc. since Vec2 and Vec3 have more broader use cases then just to define the coordinate system.
Additionally, I do think it may be beneficial to keep a typedef ECEFCoordinates because I believe it would be confusing if we are converting between multiple types of coordinate points and all of them are defined with a Vec3.
|
It might be better to replace celestial coordinate with equatorial coordinate since equatorial coordinates are a specific definition of celestial coordinates. |
|
It seems that you changed celestial to equatorial in your PR, so I won't implement that here to prevent merge conflicts in the future |
385ce43 to
1d8013f
Compare
Description
Resolves #44
Adds a time flag request to found's distance command that marks when the image was taken (in nanoseconds). This allows for a more accurate understanding of the satellite's latitude and longitude. Currently outputs time into the outputted .found file.
This PR also modifies the output of output.cpp's GetEarthCoordinates function to be ECEF coordinates. This is the standard set for representing position by NASA (example: https://ntrs.nasa.gov/api/citations/20190002515/downloads/20190002515.pdf) and other space grade receivers.
Note that the nanoseconds since epoch is being stored as an unsigned 64-bit integer throughout the code. This can reliably store up until the year 2557.
Artifacts for PR #60 (DO NOT CHANGE)