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

More Filter Changes #108

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

More Filter Changes #108

wants to merge 13 commits into from

Conversation

jpanikulam
Copy link
Member

No description provided.

@jpanikulam jpanikulam force-pushed the more_filter_world branch 3 times, most recently from 1390815 to bf1ab95 Compare March 30, 2019 18:21
// const auto time_of_validity = to_time_point(imu_msg.timestamp);
// const jcc::Vec3 gyro_radps(imu_msg.gyro_radps_x, imu_msg.gyro_radps_y, imu_msg.gyro_radps_z);
// estimation::jet_filter::GyroMeasurement gyro_meas;
// gyro_meas.observed_w = gyro_radps;
Copy link
Contributor

Choose a reason for hiding this comment

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

//screams

sensor_geo_->add_ellipsoid({fit.ellipse, jcc::Vec4(0.4, 0.6, 0.4, 0.7), 2.0});
sensor_geo_->flush();
};
// std::cout << "Optimizing" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

// std::cout << screams


// speeds_plot.subplots["opt_est_dot_x"].points.push_back({dt, x.x.R_world_from_body.log().x()});
// speeds_plot.subplots["opt_est_dot_y"].points.push_back({dt, x.x.R_world_from_body.log().y()});
// speeds_plot.subplots["opt_est_dot_z"].points.push_back({dt, x.x.R_world_from_body.log().z()});
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments before landing

@jpanikulam jpanikulam force-pushed the more_filter_world branch 3 times, most recently from 9438487 to 7b2d67c Compare July 7, 2019 23:15
Copy link
Contributor

@IJDykeman IJDykeman left a comment

Choose a reason for hiding this comment

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

will review the rest later

@@ -2,7 +2,8 @@ serial_number: 31E7DB5E
v4l_path: usb-046d_Logitech_Webcam_C930e_31E7DB5E-video-index0
note: these cal values were copied from camera 0
camera_matrix: [499.7749869454186, 0, 309.3792706235992,
0, 496.9300965132637, 241.6934416030273,
0, 496.9300965132637, 241.6934416030273,
Copy link
Contributor

Choose a reason for hiding this comment

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

noble

JetVaneMapper mapper_;

const jcc::Vec3 w_jet_frame = pose.world_from_jet.inverse() * pose.w_world_frame;

const MatNd<3, 3> Kp = cfg.kp.asDiagonal();
Copy link
Contributor

Choose a reason for hiding this comment

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

3.6 roentgen/hr on this abbreviation

@@ -8,6 +8,11 @@
namespace jet {
namespace control {

struct GainConfig {
jcc::Vec3 kp;
Copy link
Contributor

Choose a reason for hiding this comment

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

expand acronym

node["camera_matrix"].push_back(K.at<double>(i, j));
}
}
node["resolution"].push_back(cols);
Copy link
Contributor

Choose a reason for hiding this comment

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

low nobility factor maintaining this variable as state while iterating over images.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait -- what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

referring to the fact that we set cols = image->image.cols; above as a way of getting the resolution for all the images. Could improve by either giving a more specific name so that once doesn't have to scroll way up to see what it refers to. Or by breaking all the image reading into a separate function that returns a struct that keeps all the data you get from reading images together.

jcc::Warning() << "Calibrating camera, this might take a while..." << std::endl;
int flag = 0;
const auto t0 = jcc::now();
cv::calibrateCamera(object_points, image_points, cv::Size(480, 270), K, D, rvecs, tvecs, flag);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pull out resolution constant

}

jcc::Optional<ejf::FiducialMeasurement> find_nearest_fiducial(
const std::vector<estimation::TimedMeasurement<ejf::FiducialMeasurement>> fiducial_meas,
Copy link
Contributor

Choose a reason for hiding this comment

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

meas =/= measurements

std::cout << node << std::endl;
}

jcc::Optional<ejf::FiducialMeasurement> find_nearest_fiducial(
Copy link
Contributor

Choose a reason for hiding this comment

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

call it eg find nearest in time

return SE3(world_from_camera, jcc::Vec3::Zero());
}

ejf::Parameters compute_parameters(
Copy link
Contributor

Choose a reason for hiding this comment

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

better name than compute_parameters

@@ -24,7 +24,8 @@ struct VaneConfiguration {
SE3 T_vane_unit_from_vane_default = detail::compute_T_vane_unit_from_vane_default();
double offset_vane_from_servo_rad = 0.0;

double gear_ratio_servo_from_vane = 5.33;
// double gear_ratio_servo_from_vane = 5.33;
double gear_ratio_servo_from_vane = 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to be the actual ratio. Are we sure we should land this?

@@ -16,39 +18,46 @@
namespace jet {

void ServoBq::init(const Config& config) {
subscriber = make_subscriber("servo_command_channel");
subscriber_ = make_subscriber("servo_command_channel");
publisher_ = make_publisher("servo_pwm_commands_channel");
Copy link
Contributor

Choose a reason for hiding this comment

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

I might indicate in the subscriber and publisher names the channels they are on.

void ServoDriver::set_percentage(float unchecked_percentage) {
if (unchecked_percentage > 100 || unchecked_percentage < 0) {
void ServoDriver::set_percentage(const double unchecked_percentage, const int max_pwm_count, const int min_pwm_count) {
if (unchecked_percentage > 1.0 || unchecked_percentage < 0.0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you are calling this "percentage" but it is expected to be in [0,1] not [0,100]. Rename?

// Set up the viewer
//

setup();
Copy link
Contributor

Choose a reason for hiding this comment

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

call it eg setup_viewer

constexpr int DECIMATION = 25;

int i = 0;
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might break this into a function that returns in the data you need from the images.

node["camera_matrix"].push_back(K.at<double>(i, j));
}
}
node["resolution"].push_back(cols);
Copy link
Contributor

Choose a reason for hiding this comment

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

referring to the fact that we set cols = image->image.cols; above as a way of getting the resolution for all the images. Could improve by either giving a more specific name so that once doesn't have to scroll way up to see what it refers to. Or by breaking all the image reading into a separate function that returns a struct that keeps all the data you get from reading images together.

jf.measure_fiducial(fiducial_meas.measurement, fiducial_meas.timestamp);
}

constexpr bool USE_ACCELEROMETER = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems too important to be floating down here.

collected_fiducial_meas.push_back({fiducial_measurement, t});
}

// TODO: NEXT, FILL THIS IN
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray comment?


const std::vector<std::string> imu_channels = one_imu_present ? single_imu : both_imu;

// const bool two_imus = !one_imu_present;
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete

gonogo().nogo("Starting up");
std::cout << "Starting up" << std::endl;

std::cout << "Subscribing Fiducial..." << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove prints


pose_geo_->add_axes({pose.world_from_jet, 0.55, 3.0, true});

// const jcc::Vec3 jet_origin(1.0, 0.0, 0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete comments

pose_geo_->flip();

const SE3 body_from_model = jcc::exp_z(-M_PI * 0.5);
// const SE3 body_from_model = jcc::exp_z(0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete comment

@@ -9,6 +9,13 @@

namespace jet {

//
// Pose has the following contract:
// - Z points in the gravity direction
Copy link
Contributor

Choose a reason for hiding this comment

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

Might disambiguate by writing +Z here since it looks like -Z by accident.

std::cerr << channel_name_
<< ": Not connected. Skipping read. Reason: " << (connecting_ ? "Connecting" : "Not connected")
<< std::endl;
// std::cerr << channel_name_
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and the one above

@IJDykeman IJDykeman closed this Aug 21, 2019
@IJDykeman IJDykeman reopened this Aug 21, 2019
@IJDykeman IJDykeman self-requested a review August 21, 2019 03:28
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.

3 participants