ZendarInc / ZendarSDK

MIT License
12 stars 6 forks source link

First version of the raw radar data reader #22

Closed antoniopug closed 3 years ago

antoniopug commented 3 years ago

I tested the example script with acquisition /mnt/zen-ffa/nyquist/2020-12-04_0048/000 and calibration file ZenCode/ZenModel/radar/antenna/intrinsic/ZRCA0003.pt.

The things I would appreciate looking at are:

osamasal commented 3 years ago

@antoniopug one issue that @plb9772 brought up is that we have moved all protos to ZenCode and we're in the process of publishing public protos via .deb packages in ZendarSDK. This PR adds protos back to ZendarSDK, but it would be best if we rely on the .deb packages to distribute the protos. If this PR can wait for the .deb package to be merged into ZendarSDK, then that would be best, otherwise, we can always remove them later after the .deb packages are merged into ZendarSDK

antoniopug commented 3 years ago

@osamasal yeah would love to move over to the deb thing asap. When is that expected to land?

plb9772 commented 3 years ago

I know I was not asked to review this but I'd like to throw some thoughts here:

This is just some quick thoughts after reading quickly this PR, I might miss some broader picture here.

antoniopug commented 3 years ago

@plb9772 Thanks for your comments. I agree with what you're saying, and I'm not that happy at doing this tool this way.

The goal here was to quickly get this particular tool out the door to one collaborating group (U-Alabama). We need to have a discussion around what are the anticipated use cases for collaborating with external groups, and what is the right architecture for ZendarSDK to best support those use cases that we choose. In the absence of that clarity and infrastructure, I had to just build a first pass at this that is very specific to one particular use case.

osamasal commented 3 years ago

@osamasal yeah would love to move over to the deb thing asap. When is that expected to land?

Should be very soon. I have the PR out for review and I'm just waiting for sign-off before merging. In this PR, I updated the CPP examples, but I'll update and fix the existing Python tools under ZendarSDK/python/viewer shortly after. My ETA for merging this PR is early week of 03/07/2021.

antoniopug commented 3 years ago

Rebased on main to take in the deb packages. Will now work those in.

antoniopug commented 3 years ago

@osamasal I removed the proto files, updated the README to use the build in ZendarSDK/cpp/, and updated the environment.sh script as well.

I tested this by zero-ing out my PYTHONPATH and following the steps in the README, and I was able to run the example.py script without any issues. For the --calibration flag you can point it to ZenCode/ZenModel/radar/antenna/intrinsic/ZRCA0003.pt

Can you review the changes and re-try the build from scratch to check that it all works?

osamasal commented 3 years ago

@osamasal I removed the proto files, updated the README to use the build in ZendarSDK/cpp/, and updated the environment.sh script as well.

I tested this by zero-ing out my PYTHONPATH and following the steps in the README, and I was able to run the example.py script without any issues. For the --calibration flag you can point it to ZenCode/ZenModel/radar/antenna/intrinsic/ZRCA0003.pt

Can you review the changes and re-try the build from scratch to check that it all works?

Will do!

antoniopug commented 3 years ago

I moved raw_radar_reader under python per PL's suggestion. Also adding him as a reviewer.

I have tested the new instructions and they seem to work.

osamasal commented 3 years ago

@osamasal I removed the proto files, updated the README to use the build in ZendarSDK/cpp/, and updated the environment.sh script as well. I tested this by zero-ing out my PYTHONPATH and following the steps in the README, and I was able to run the example.py script without any issues. For the --calibration flag you can point it to ZenCode/ZenModel/radar/antenna/intrinsic/ZRCA0003.pt Can you review the changes and re-try the build from scratch to check that it all works?

Will do!

OK, I just tried your instructions and all looks good. I got a bunch of these warning messages, but I assume this is OK:

WARN: Frame out of sync:  22782
WARN: Frame out of sync:  22782
WARN: Frame out of sync:  22782
WARN: Frame out of sync:  22782
WARN: Frame out of sync:  22782
WARN: Frame out of sync:  22782
WARN: Frame out of sync:  22782

Also, I'm attaching the image that came up after running the command on the zen-ffa/nyquist/2020-12-04_0048/000 data set. Screenshot from 2021-03-10 11-43-37

antoniopug commented 3 years ago

@osamasal that all looks correct, thanks for your help reviewing! I think that with the repo refactoring I did this morning (at yours and PL's suggestion), it's looking like a lot cleaner integration of this tool.

osamasal commented 3 years ago

@osamasal that all looks correct, thanks for your help reviewing! I think that with the repo refactoring I did this morning (at yours and PL's suggestion), it's looking like a lot cleaner integration of this tool.

:+1: