Skip to content
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

consistent notation #116

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

consistent notation #116

wants to merge 7 commits into from

Conversation

otim
Copy link
Contributor

@otim otim commented Jun 8, 2015

No description provided.

* state.Get<StateLIdx>()(0); // p

H.block<3, 3>(0, kIdxstartcorr_q) = -C_wv * C_q * pci_sk
H.block<3, 3>(0, kIdxstartcorr_q) = -C_wv.transpose() * C_wi * pci_sk
* state.Get<StateLIdx>()(0); // q

H.block<3, 1>(0, kIdxstartcorr_L) =
scalefix ?

Choose a reason for hiding this comment

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

can you also fix this horrible ? : syntax ?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you do so, please also change below:

if (scalefix) {
  H.block<3, 1>(0, kIdxstartcorr_L).setZero();
} else {
  H.block<3, 1>(0, kIdxstartcorr_L) = ...;
}

@simonlynen
Copy link
Contributor

@otim thanks for the nice cleanup. lgtm.

@otim
Copy link
Contributor Author

otim commented Jun 8, 2015

I found another occurence of Eigen::Matrix<double, 3, 3> C_q = state.Get<StateDefinition_T::q>().conjugate().toRotationMatrix(); with the conjugate in spherical_measurement.h... Probably we should change it there as well, but I did not feel like it just now. This is a mess :(

@otim
Copy link
Contributor Author

otim commented Jun 15, 2015

I just tested with both pose measurements and position measurements and everything still looks good. please take a look at the latest changes and pay extra attention to things related to spherical measurements and pressure sensor, as I am not able to test these. Thanks!

@@ -170,16 +170,16 @@ class PoseSensorManager : public msf_core::MSF_SensorManagerROS<
P.setZero(); // Error state covariance; if zero, a default initialization in msf_core is used

p_vc = pose_handler_->GetPositionMeasurement();
q_cv = pose_handler_->GetAttitudeMeasurement();
q_vc = pose_handler_->GetAttitudeMeasurement();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this? At least vicon publishes p_vc and q_cv

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we need to measurement_world_sensor parameter here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that! For me this was the case, so this notation made it easier to understand for me... Simon, can you please elaborate on your idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on the subscribed topic (ptam/vicon) the pose provided can be
either q_cv (ptam) or q_vc/q_wc (vicon). If you look at the implementation
of the pose update you see that there is a fix-parameter (from yaml) that
defines if the pose measurement needs to be inverted or not before being
applied as update. You would need to add the same logic here.

On Tue, Jun 16, 2015, 09:39 Tim Oberhauser [email protected] wrote:

In msf_updates/src/pose_msf/pose_sensormanager.h
#116 (comment):

@@ -170,16 +170,16 @@ class PoseSensorManager : public msf_core::MSF_SensorManagerROS<
P.setZero(); // Error state covariance; if zero, a default initialization in msf_core is used

 p_vc = pose_handler_->GetPositionMeasurement();
  • q_cv = pose_handler_->GetAttitudeMeasurement();
  • q_vc = pose_handler_->GetAttitudeMeasurement();

Sorry about that! For me this was the case, so this notation made it
easier to understand for me... Simon, can you please elaborate on your idea?


Reply to this email directly or view it on GitHub
https://github.com/ethz-asl/ethzasl_msf/pull/116/files#r32497243.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please have a look at my latest commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, the first attitude measurement was always added inversed in the case of vicon... Ok, vicon measurements are added at a high rate so the effect was maybe never visible...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it says that measurement_world_sensor is false for ptam:
http://wiki.ros.org/ethzasl_sensor_fusion/Tutorials/sfly_framework

otim added 2 commits June 22, 2015 17:10
…vc or q_cv. added a check during initialization to distinguish between the two cases
Conflicts:
	msf_updates/src/pose_msf/pose_sensormanager.h
	msf_updates/src/pose_pressure_msf/pose_pressure_sensormanager.h
	msf_updates/src/position_msf/position_sensormanager.h
	msf_updates/src/position_pose_msf/position_pose_sensormanager.h
@simonlynen
Copy link
Contributor

@burrimi could you take a look at getting this one into master after testing?

@v0n0
Copy link

v0n0 commented Jan 22, 2016

+1

@v0n0
Copy link

v0n0 commented Jan 29, 2016

What would it take to test this one?

@v0n0
Copy link

v0n0 commented Apr 7, 2016

@marija-p I saw that you recently took on the task of merging PRs, do you think you could also merge this one? Thanks!

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

Successfully merging this pull request may close these issues.

6 participants