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

Changes for new Location Tracking #64

Merged
merged 30 commits into from
Feb 8, 2025
Merged

Changes for new Location Tracking #64

merged 30 commits into from
Feb 8, 2025

Conversation

cgpadwick
Copy link
Collaborator

No description provided.

@cwosw cwosw changed the title Bash changes for new Location Tracking Changes for new Location Tracking Jan 25, 2025
@cwosw
Copy link
Collaborator

cwosw commented Jan 25, 2025

We still need to get the cuda code synced up to this.

Module_YL20230518V0 TEMPLATE
UC762 TEMPLATE
UC7626 TEMPLATE
UC762W TEMPLATE
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can all be changed, its just that TEMPLATE is the current file that has something in it, so that is where it will lie as a default.

@cwosw cwosw marked this pull request as ready for review January 27, 2025 02:17
@cwosw cwosw self-assigned this Jan 27, 2025
@cwosw
Copy link
Collaborator

cwosw commented Jan 27, 2025

The code now does actually work. I used the wrong names.

@cwosw
Copy link
Collaborator

cwosw commented Jan 28, 2025

Pardon the many many commits, this is what happens when you repeat confidence in a fix that does not work. Such is the life of a Bash dev. This works. It is tested on the Orin.

@@ -20,23 +20,20 @@ function killIfRunning() {
}

# set clocks to max frequency (jetson only)
jetson_clocks || true
jetson_clocks || true 1>&2 >> /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

The weird Bash redirection things basically boil down to "Take the errors, put them into standard error, then throw the entire thing into /dev/null."

Copy link
Collaborator Author

@cgpadwick cgpadwick left a comment

Choose a reason for hiding this comment

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

overall looks really good, just address a few minor things and it's good to go. Also I can't approve since apparently it's my PR.


# ensure that the calibration file exists
if ! [[ -f /apps/AprilTags/data/calibration/calibrationmatrix_${camID}.json ]]; then
if ! [[ -f /opt/AprilTags/data/calibration/calibrationmatrix_${camID}.json ]]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice error checking here, with clear, understandable error messages! Great job!

# now onto the AprilTags specific items
cp -R AprilTags/ /apps/
cp -R bin/* /apps/bin/
systemctl enable AprilTagsPipeline.service # not now, since files might not be there
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused by the comment. Can you remove it or modify it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is me being confused myself. I will better implement this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right. "systemctl enable --now" is what I was referencing. the not "now" comment was meant as a "enable the service, but do not start it now" type comment. Badly worded, I will fix that.

@@ -248,27 +248,5 @@
36.73500062867949
]
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how do you feel about deleting all these wrong calibration matrix files now that we fixed the calibration bug? It would force us to recalibrate the cameras and check in the files which is a good thing. I was going to put a separate PR for that but we could do it in this one if you wanted to. @cwosw let me know

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be helpful to re calibrate, but I am hesitant to remove these files from this PR (I personally feel that it would be better for the new calibration data to either be put into this PR tomorrow or to make a new PR for it).

@@ -0,0 +1,24 @@
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confirmed rotation correct for back camera

Copy link
Collaborator

Choose a reason for hiding this comment

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

To this and the ones below it, does that mean that these are working and good?

@@ -0,0 +1,24 @@
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confirmed rotation correct for front camera

@@ -0,0 +1,24 @@
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confirmed rotation correct for front cam

src/ws_server.cu Outdated
@@ -125,11 +129,14 @@ class AprilTagHandler : public seasocks::WebSocket::Handler {
});
}

bool parsecal_file(const std::string& cal_filepath,
bool parsecal_file(const std::string& cal_filepath, // why specify this if it is not once touched?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the comment doesn't make sense, please remove. Was it referring to an intermediate version of the file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a reference to the function not at all using the "cal_filepath" parameter when I was working with it. I made it use the passed argument since.

src/ws_server.cu Outdated
}

if (FLAGS_camera_name.empty()) {
LOG(ERROR) << "camera_name is required";
return 1;
}
gflags::ParseCommandLineFlags(&argc, &argv, true);
//gflags::ParseCommandLineFlags(&argc, &argv, true); // this is done once before
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, just remove this line

src/ws_server.cu Outdated

if (!std::filesystem::exists(FLAGS_cal_file)) {
LOG(ERROR) << "calibration file does not exist: " << FLAGS_cal_file;
return 1;
}

if (!std::filesystem::exists(FLAGS_position_file)) {
LOG(ERROR) << "position files does not exist: " << FLAGS_position_file;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typo: files -> file

src/ws_server.cu Outdated
FLAGS_rotate_vertical,
FLAGS_rotate_horizontal);
int port = 8080;
int port; // why reassign it?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

stale comment, will make no sense to future viewers of the code, please remove it

@cwosw
Copy link
Collaborator

cwosw commented Feb 5, 2025

You made the PR, I can approve it once you say that it works good.

@@ -1,14 +1,14 @@
{
"matrix": [
[
911.4344011282251,
1289.5969133704853,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this doesn't look right to me. @cwosw can you recalibrate this one and see if the numbers change?

Copy link
Collaborator Author

@cgpadwick cgpadwick left a comment

Choose a reason for hiding this comment

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

3 cameras have incorrect calibration coeffs

1298.2332221828685,
533.3445083443279
942.085983323447,
529.1941472556863
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

very suspicious

@@ -1,14 +1,14 @@
{
"matrix": [
[
902.6757273943186,
5804.5438064798545,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no way this is right

@@ -1,14 +1,14 @@
{
"matrix": [
[
959.8481551321847,
1121.9157012370742,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no way this is right

Copy link
Collaborator Author

@cgpadwick cgpadwick left a comment

Choose a reason for hiding this comment

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

looks good, let's merge!

Copy link
Collaborator

@cwosw cwosw left a comment

Choose a reason for hiding this comment

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

Works!

@cwosw cwosw merged commit 436f81a into main Feb 8, 2025
1 check passed
@cwosw cwosw deleted the betterLocations branch February 8, 2025 21:00
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