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

GPS and Drone Modules #1

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

GPS and Drone Modules #1

wants to merge 14 commits into from

Conversation

ArjunGupte1
Copy link
Collaborator

Added all new files and modifications to old files for the GPS and drone modules (on the C# side).

Copy link
Contributor

@MatthewCalligaro MatthewCalligaro left a comment

Choose a reason for hiding this comment

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

Great work! Most of the comments are relatively small specific changes. We can discuss the timeout and image logic refactoring when we call later this week.

@@ -159,6 +159,11 @@ private enum Header
lidar_get_samples,
physics_get_linear_acceleration,
physics_get_angular_velocity,
physics_get_position,
drone_get_drone_image,
drone_get_drone_height,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make get_drone_height vs set_height consistent. I'm personally a fan of just drone_get_image and drone_get_height to be concise and avoid repetition.

@@ -40,7 +40,7 @@ public class PythonInterface
/// <summary>
/// The time (in ms) to wait for Python to respond.
/// </summary>
private const int timeoutTime = 5000;
private const int timeoutTime = 20000; //changed this from 5000 to 20000 since image processing takes > 5s
Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, it is a bad user experience to have something to go wrong and hang 20 seconds before seeing an error. Let's discuss this in person and find a way to keep the timeout at 5 seconds. We should be running at ~60 frames per second, so a 5 second timeout is already extremely generous.

/// </summary>
private bool mustUpdateDroneImageRaw;

private void UpdateDroneImageRaw()
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot of duplicated logic between Drone.cs and CameraModule.cs. Can we factor out the duplicate logic into a utils class instead?

/// The average relative error of position measurements.
/// This value is made up (it is NOT specified in the Intel RealSense D435i datasheet).
/// </summary>
private const float positionErrorFactor = 0.001f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering: where did you get these numbers from?

/// The average fixed error applied to all position measurements.
/// This value is made up (it is NOT specified in the Intel RealSense D435i datasheet).
/// </summary>
private const float positionErrorFixed = 0.005f; //old value => 4.90f
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did the old value (4.90f) come from?

@@ -82,6 +94,32 @@ public Vector3 AngularVelocity
return this.angularVelocity.Value;
}
}

/// <summary>
/// The position of the car (in meters).
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the summary to clarify "position relative to what"

{
if (!this.position.HasValue)
{
// Unity uses a left-handed coordinate system, but our API is right-handed
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@@ -72,6 +72,11 @@ public class Racecar : MonoBehaviour
/// The heads-up display controlled by this car, if any.
/// </summary>
public Hud Hud { get; set; }

/// <summary>
/// Exposes the drone model.
Copy link
Contributor

Choose a reason for hiding this comment

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

Go ahead and move this above the Hud so it is right after the PhysicsModule (to keep the modules together)

@@ -40,7 +40,7 @@ public class PythonInterface
/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Above, you need to change private const int version = 1; to 2 because you are updating the communication protocol

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.

2 participants