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.

Sign up for GitHub

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

Jump to bottom

add support for pblog #140

Merged
merged 9 commits into from
May 29, 2023
Merged

add support for pblog #140

merged 9 commits into from
May 29, 2023

Conversation

andermi
Copy link
Collaborator

@andermi andermi commented Apr 12, 2023 •
edited
Loading

added to batching and standard launch

requires:
osrf/mbari_wec_utils#44 <-- osrf/mbari_wec_utils#47

@andermi
add support for pblog
6668332 
Signed-off-by: Michael Anderson <[email protected]>
@andermi andermi mentioned this pull request Apr 12, 2023
Merged
@mabelzhang mabelzhang linked an issue Apr 17, 2023 that may be closed by this pull request
Log files #114
Closed
@hamilton8415
Copy link
Collaborator

Looking good. Could consider changing "latest_csv" to "latest" to match buoy behaviors.

@andermi
Copy link
Collaborator Author

andermi commented May 3, 2023 •
edited
Loading

consider changing "latest_csv" to "latest"

I can do that, but I was hoping to distinguish between pblog and rosbag. No biggie if you'd like to have latest pointing to the csv and perhaps latest_rosbag pointing to the rosbag

@hamilton8415
Copy link
Collaborator

I'm flexible. Nothing automated relies on this and it's just a convenience so it can stay as is.

In general, I noticed a few things when I was running this yesterday (I was running from batch mode)

  1. When the job started there was immediately a .gz version of the .csv file, even though the .csv file was still growing. Same filename.
  2. When it completed, the "latest" file pointed to the .gz file. That is OK, but not really necessary. For me anyway, I've been using the "latest" links as a quick way to see what's happening during a run (or a deployment).

@andermi
Copy link
Collaborator Author

andermi commented May 3, 2023

  1. Fixed
  2. would you rather not gzip the log on exit? (only after a duration)

@hamilton8415
Copy link
Collaborator

Yeah, I think we should match the buoy, zip only at the end of an hour...

@andermi
Copy link
Collaborator Author

andermi commented May 3, 2023

it was causing problems anyways... if it takes too long to gzip, the batch launch will SIGTERM/SIGKILL pblog before it finishes writing

@andermi
Copy link
Collaborator Author

andermi commented May 3, 2023

@hamilton8415 ready for another check

@hamilton8415
Copy link
Collaborator

In testing this I realized the wave-seed wasn't getting passed along by IncidentWave.cpp. Fixed this and pushed to this branch...

@andermi
Merge branch 'main' into andermi/launch_sim_pblog
100d196
@andermi
Fix logging rates (#146)
6091a28 
* add sim-time-aware Rate to controllers for publishing

Signed-off-by: Michael Anderson <[email protected]>

* forgot shutdowns to stop hanging

Signed-off-by: Michael Anderson <[email protected]>

* add copyright

Signed-off-by: Michael Anderson <[email protected]>

* linters

Signed-off-by: Michael Anderson <[email protected]>

* add async proxy to await future for pack rate calls (#147)

Signed-off-by: Michael Anderson <[email protected]>

* use sim time from postupdate instead of imu time

Signed-off-by: Michael Anderson <[email protected]>

* enable use_sim_time for AHRS

Signed-off-by: Michael Anderson <[email protected]>

---------

Signed-off-by: Michael Anderson <[email protected]>
@andermi
linters
1d3773c 
Signed-off-by: Michael Anderson <[email protected]>
hamilton8415
hamilton8415 approved these changes May 16, 2023
View reviewed changes
Copy link
Collaborator

@hamilton8415 hamilton8415 left a comment

Choose a reason for hiding this comment

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

I have used this feature quite a bit in the last week or so and it's working very nicely. The only changes here I might suggest is to rename the "latest_csv" symbolic link to "latest_csv_dir". I kept thinking the link pointed at the latest .csv file... But I got used to it. Not a critical change.

The other hiccup I've found is maybe not due to this PR but rather the logging PR in _gz. When performing runs longer than one-hour, it zips the last file correctly, but there does seem to be a gap in results between the two files, when running much faster than real-time the gap is greater than two minutes of sim time... I will note this in the _gz pull request also.

@andermi
Copy link
Collaborator Author

andermi commented May 16, 2023

rename the "latest_csv" symbolic link to "latest_csv_dir"

changed in osrf/mbari_wec_utils#44

@andermi
Copy link
Collaborator Author

andermi commented May 16, 2023

gap in results between the two files

fixed in osrf/mbari_wec_utils#44

@andermi
Copy link
Collaborator Author

andermi commented May 17, 2023

@hamilton8415 can you try your matlab stuff against this latest patch to confirm the new IMU orientation?

@hamilton8415
Copy link
Collaborator

Lat/Lon working now.

@hamilton8415
Copy link
Collaborator

I have done further testing for this and believe it is ready to be merged into main, along with mbari_wec_utils #44

@andermi andermi merged commit d572280 into main May 29, 2023
@andermi andermi deleted the andermi/launch_sim_pblog branch May 29, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamilton8415 hamilton8415 hamilton8415 approved these changes

@rhenthorn rhenthorn Awaiting requested review from rhenthorn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Log files
2 participants
@andermi @hamilton8415