Skip to content
This repository was archived by the owner on Jan 23, 2024. It is now read-only.

Feature/time sync #3

Closed
wants to merge 8 commits into from
Closed

Feature/time sync #3

wants to merge 8 commits into from

Conversation

raghavkhanna
Copy link
Collaborator

Added hardware time stamping and RC based roll/pitch/yawrate/thrust control
@inkyusa @marija-p

Copy link

@HannesSommer HannesSommer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The experimental introduction of the cuckoo time translator doesn't seem to be properly separated from other commits. I'd recommend splitting that properly so you can revert this easily in case problems do occur, e.g. during the integration week..

if(bc_data.timeStamp.nanoTime < ::g_prevNanoTime){
::g_counter++;
}
g_hardwareStamp = ::g_counter*::g_WRAP_TIME + double(bc_data.timeStamp.nanoTime)*(1e-9);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cuckoo_time_translator packages provides a class that does this for you. It is also used within the update call up in line 64. If the nanoTime proves to be a proper wrapping hardware clock you can just use it instead of bc_data.timeStamp.time in line 64 - of course you would need to update the information given to the constructor of the device_time_translator.

dji_sdk::A3GPS A3_GPS;
dji_sdk::A3RTK A3_RTK;
// Declare Time translator
boost::shared_ptr<cuckoo_time_translator::DeviceTimeUnwrapperAndTranslator> device_time_translator_; // TODO: Why does this have to be a boost::shared_ptr ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boost::shared_ptr is not needed. In the example driver this was the simplest available solution (pre c++11) to the deferred initialization requirement that came with the fact that the node handle was it requires was created in the onInit method.
As your class gets the node handle from outside passed to the constructor you don't need a deferred initialization at all and could make this a simple member and move its initialization to the initialization list of the constructor.

@@ -50,10 +52,19 @@ void DJISDKNode::broadcast_callback()
auto current_time = ros::Time::now();



Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check that bc_data doesn't have a receive time stamp? If it was high quality it clearly would..
If not consider moving up the ros::Time::now() call right below the getBroadcastData() call.

@raghavkhanna
Copy link
Collaborator Author

@HannesSommer thanks for the review, I'll incorporate the changes before merging to master :)

@HannesSommer
Copy link

@raghavkhanna , wait with merging for this PR to settle: ethz-asl/cuckoo_time_translator#5. There are also some new changes (last commit) that I introduced to go better in line with the new drivers I saw. As a blueprint for the necessary adaptations : ethz-asl/pointgrey_camera_driver@5dac172.

@raghavkhanna
Copy link
Collaborator Author

Superseded by #8. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants