Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/time source #57
Feature/time source #57
Changes from 25 commits
22f8275
160e8d5
aa24677
870f189
711109a
4a01acf
b1eccdd
ce6e7ee
02370bc
fb35e0d
d90013a
3d1b9dc
6eb30bb
7417127
d0d763d
9d5cbff
9fb6b5a
6f4a1f7
f62d820
d500a6d
c4b8f06
e92f7f5
f9f5697
d2579c8
7b1f836
ced9ff0
2937c05
0170b5d
60633c8
de2170a
79c3315
de7fd12
6616549
8d2d5e3
a2c1965
3e09834
4778fe4
8a65604
5103d16
c788b25
74148ef
4f464dc
1f8235d
1acae3c
713fe56
340fee9
8e28d16
32b442a
d9a47fa
b91b142
b48a1c4
2759f50
6de48c0
792f9f5
311cf83
81cf059
c350996
9c09200
6a4dd17
f5024cc
953d2f5
fb53afe
599eb98
c56b8e1
e03b854
2750614
0b1edd1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not installing proj from apt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CARMA Cloud is 6 years old and may not have had the option of a DockerHub image or apt at that time. We'll need to verify that the required headers are included to compile the JNI wrapper for proj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not installing JDK from apt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that the apt version does not include the JNI header files needed to compile cs2cs library. Continuing to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using gcc directly instead of a build system like CMake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the original author thought CMake was overkill for 3 includes and 1 source file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using javac directly instead of a build system like maven?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be doing substitutions on configuration files during our build. Instead you can use environment variables and or build arguments or simply setup a volume for the configuration files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a hard coded traffic control at 0 seconds? Does this not break real world functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be replaced with : clock:now()->either simulation time or real time based on configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace all calls to System.time() with TimeSource so that
lNow
can still be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses singleton TimeSource now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use ambassor config to indicate simulation mode. Please have a separate configuration parameter to be more explicit. Service should fail of sim mode is true and ambassador is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added simulation configuration parameter and updated SimFederate to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this id configurable and can we use a JSON library to serialize JSON strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CARMA Cloud id added to configuration and registration message uses JSON library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be replaces with a call to time wrapper that either returns sim time or real time base on configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use TimeSource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is a commit I am missing. Please make sure you pull and push you local changes.