CV Pipeline Integration #73

atar13 · 2024-01-05T09:02:28Z

Closes #62
Closes #30

Changes

Set up a skeleton of the Computer Vision pipeline. This includes classes and function signatures for all modules of the pipeline. Now, the types of output data of each stage of the pipeline is well defined.

Also created a SearchState class that inherits from MissionState. In its tick function it starts up a new thread that captures a new image from the camera and passes it into the pipeline. I have not implemented any way to collect results from multiple threads, send the results to the GCS, or shut down threads that exceed a timeout. These should be done in a later PR.

This PR also includes a MockCamera implementation that implements CameraInterface. I just added it to easily be able to fetch dummy image data. Right now it just returns an image with a solid color but in the future it should return an image with valid competition targets.

Testing

There's a small unit test to test an image crop helper function. tests/unit/cv/utilities_test.cpp

There's another unit test to test the mock camera test/unit/camera/mock.cpp

There's an integration test to allow someone to run the entire CV pipeline using an arbitrary image from a filepath. tests/integration/pipeline.cpp

Feedback

Any feedback is appreciated, but I'd like to focus on a few specific parts.

I tried to implement some version of dependency injection with the CameraInterface. It can be found here based on the first approach in this blog post.
I also would like some feedback on how I'm spawning the pipeline thread and the comments right above about thread safety https://github.com/tritonuas/obcpp/blob/feat/cv-orchestrator/src/core/states.cpp#L48-L81.
Also if there's a better way to represent the results of the Pipeline, I'd like to hear it.
Any other C++ idioms/standards I violated especially with any OOP stuff

atar13 · 2024-01-05T09:03:49Z

Going to also ping @dzodkin33 since I touched some of the CameraInterface code to get the MockCamera up and running.

Also want to ping @hashreds and @cr1t1cql since they will be filling in some of the modules of the pipeline.

empty classes/methods for CV pipeline. this commit will define the types and layout of inputs/outputs of each stage of the pipeline. includes the following modules: - saliency - localization - matching - segmentation - classification also added some additional data to ImageData that will be needed for pipeline.

right now only testing crop function

implements two methods of CameraInterface to quickly plug in something to the rest of the pipeline. For now it just generates an image of a solid color. In the future we could have it return an actual competition image with a valid target that the models can recognize. set up other CameraInterface related functions to get MockCamera working

set up tick method that captures a photo and passes it through the CV pipeline in a new thread

integration test to allow for portable testing of the entire CV pipeline using an arbitrary image as input

to match other unit test files

Samir-Rashid · 2024-01-15T04:19:39Z

I have rebased and gotten the ✅ on the commit.

I have not implemented any way to...shut down threads that exceed a timeout.

I have run into this in other places in this project. I think we should make a reusable way to add this functionality.

Samir-Rashid

Amazing job, Anthony. This is an extremely well made mega-PR that was a delight to review. I really liked the long-form comments you included explaining the different parts of the system. Your additions look good and are an important step to bringing the structure of remaining work into definition. I think this design allows for a good amount of wiggle room as the implementations find things that need to be changed.

Samir-Rashid · 2024-01-15T04:19:28Z

include/camera/interface.hpp


 #include <nlohmann/json.hpp>
 #include <opencv2/opencv.hpp>

+// class to contain all telemetry that should be tagged with an image.
+// In the future this could be in a mavlink file.


What is a mavlink file

I was thinking of a mavlink.hpp that would be added by Jasper in his Mavlink Client.

Samir-Rashid · 2024-01-15T04:23:14Z

include/camera/interface.hpp

+// In the future this could be in a mavlink file.
+class ImageTelemetry {
+ public:
+    ImageTelemetry(double latitude, double longitude, double altitude, double airspeed, double yaw,


This could use the GPSCoord type. I like how you made a ImageTelemetry type so it is easy to add/remove fields in the future. I think it is more conventional to make this a struct since everything is public. Also this should have a default initializer {} for all the values.

I like the idea of removing some of the fields in favor of GPSCoord. I have also changed ImageTelemetry to a struct.

What do you mean by default initializer? Is that the same as the constructor implementation in camera/interface.cpp? https://github.com/tritonuas/obcpp/blob/feat/cv-orchestrator/src/camera/interface.cpp#L3-L11

Yes, that will make sure this all gets zero initialized.

Samir-Rashid · 2024-01-15T04:35:57Z

include/camera/interface.hpp

@@ -50,8 +69,8 @@ class CameraConfiguration {
 class CameraInterface {
 private:
    CameraConfiguration config;
-    ImageData recentPicture;  // might need to move it to public
-    bool doneTakingPicture;      // overengineering time
+    std::unique_ptr<ImageData> recentPicture;  // might need to move it to public


Well at least it should be fine if it's private ...because the smart pointer is handling all the scoping memory for us.

// overengineering time

🤔 I cannot give useful advice on the design of this interface ...because I have no idea what kinds of functionality we have been using in the past since '87. I do not want to block based on any minor improvements, so as long as you are happy with this we should forge ahead. That being said, it would be good to reping the people in your comment to get a better set of eyes.

I should have removed that public comment. That was made by Boris in the original interface. And so was the over engineering comment. This PR wasn't supposed to drastically change the camera interface but we've already had a design talk with Boris during the meeting on how to simplify the API.

Samir-Rashid · 2024-01-15T04:40:23Z

include/core/states.hpp

+*/
+class SearchState : public MissionState {
+ public:
+    // Passing in a unique_ptr to a CameraInterface for dependency


I think your mocking approach is fine.

Samir-Rashid · 2024-01-15T04:44:22Z

include/cv/pipeline.hpp

+struct PipelineResults {
+    ImageData imageData;
+    std::vector<AirdropTarget> matchedTargets;
+    // Not sure if unmatchedTargets should hold a different struct than


For sure both should be the same type

Samir-Rashid · 2024-01-15T04:53:02Z

src/core/states.cpp

+    // Just thinking out loud regarding thread saftey here. With the code below,
+    // we will be capturing a new image from the camera and running it through
+    // the models of the CV pipeline once on every tick (every second or so).
+    // In terms of potential data races here are a few I can think of:


Simply design your system to be impossible to have data races and then put it in the appendix.

Samir-Rashid · 2024-01-15T04:54:41Z

src/core/states.cpp

+        // TODO: send to GCS for verification
+    });
+
+    // TODO: Need a way to cleanup any running threads when the state changes


This is a good point worth emphasizing. I think we can use a smart pointer for the state which will automatically call destructors for each state when needed.

Actually Tyler already brought up these concerns here #55 (comment)

tests/integration/cv_pipeline.cpp

tests/unit/camera/mock.cpp

tests/unit/cv/utilities_test.cpp

atar13 · 2024-01-17T21:38:50Z

I have rebased and gotten the ✅ on the commit.

I have not implemented any way to...shut down threads that exceed a timeout.

I have run into this in other places in this project. I think we should make a reusable way to add this functionality.

Thank you for getting the lint up to spec. Much appreciated.

* update ubuntu version 🙏 * fix groupadd command

* update ubuntu version 🙏 * fix groupadd command * fix error

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN?

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id * i am at my wits end 🔫

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id * i am at my wits end 🔫 * fix dumbass merge conflict

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id * i am at my wits end 🔫 * fix dumbass merge conflict * fix this bullshit

…l folder and test image folder

…gmentation-integration Added models folder and test images folder.

CV segmentation integration

Tyler-Lentz

The cv_pipeline integration tests looks like it is failing to compile for me:

/workspaces/obcpp/tests/integration/cv_pipeline.cpp: In function 'int main()':
/workspaces/obcpp/tests/integration/cv_pipeline.cpp:80:53: error: no matching function for call to 'Pipeline::Pipeline(std::array<Bottle, 5>&, std::vector<std::pair<cv::Mat, unsigned char> >&)'
   80 |     Pipeline pipeline(bottlesToDrop, referenceImages);
      |                                                     ^
In file included from /workspaces/obcpp/tests/integration/cv_pipeline.cpp:5:
/workspaces/obcpp/include/cv/pipeline.hpp:35:9: note: candidate: 'Pipeline::Pipeline(std::array<Bottle, 5>, std::vector<std::pair<cv::Mat, BottleDropIndex> >)'
   35 |         Pipeline(std::array<Bottle, NUM_AIRDROP_BOTTLES> competitionObjectives,
      |         ^~~~~~~~
/workspaces/obcpp/include/cv/pipeline.hpp:36:62: note:   no known conversion for argument 2 from 'vector<pair<[...],unsigned char>>' to 'vector<pair<[...],BottleDropIndex>>'
   36 |             std::vector<std::pair<cv::Mat, BottleDropIndex>> referenceImages);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
/workspaces/obcpp/include/cv/pipeline.hpp:33:7: note: candidate: 'Pipeline::Pipeline(const Pipeline&)'
   33 | class Pipeline {
      |       ^~~~~~~~
/workspaces/obcpp/include/cv/pipeline.hpp:33:7: note:   candidate expects 1 argument, 2 provided
/workspaces/obcpp/include/cv/pipeline.hpp:33:7: note: candidate: 'Pipeline::Pipeline(Pipeline&&)'
/workspaces/obcpp/include/cv/pipeline.hpp:33:7: note:   candidate expects 1 argument, 2 provided
make[3]: *** [CMakeFiles/cv_pipeline.dir/build.make:496: CMakeFiles/cv_pipeline.dir/tests/integration/cv_pipeline.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:421: CMakeFiles/cv_pipeline.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:428: CMakeFiles/cv_pipeline.dir/rule] Error 2
make: *** [Makefile:221: cv_pipeline] Error 2

Also, it would be good to change all of the couts and cerrs through the cv code to use the logging library instead.

I like the general structure of the CV stuff though. It should be easy to slot everything in where they need to go.

atar13 · 2024-03-31T03:55:26Z

cv_pipeline should compile now. We moved some stuff to use some of the protobuf structs but forgot to update it in the cv_pipeline test.

Before this gets merged in I want to

verify all the new integration tests compile
add links to download testing models/images
move cout/cerrs logging library
add comments to unit tests

@shijiew555 and @hashreds can merge in their changes to main for classification and saliency respectively. We want to get this branch merged in as soon as possible so that other branches can depend on the existing changes.

also added some comments for some of the cv test files

also removed some hardcoded model paths and moved them to constructors

we need to debug the ECEF localization algorithm but it's out of the scope of the feat/cv-orchestrator PR and I want a green checkmark

also improved error messages when model can't be loaded

Tyler-Lentz

I really like all of the code structure and organization for everything. I had the issue with the image paths, but I think that's just a me issue and I don't think its worth holding up the merge over it. Once we automate the asset pulling process fully we can rigorously test to make sure everything is robust.

atar13 requested review from Tyler-Lentz and Samir-Rashid January 5, 2024 09:02

atar13 self-assigned this Jan 5, 2024

atar13 and others added 9 commits January 15, 2024 03:47

added cv utilities tests

01b63fc

right now only testing crop function

SearchState

94070c4

set up tick method that captures a photo and passes it through the CV pipeline in a new thread

unit test mock camera

dff1aee

cv pipeline integration test

dc4fdd8

integration test to allow for portable testing of the entire CV pipeline using an arbitrary image as input

moved pathing unit tests to their own folder

952e020

to match other unit test files

linter: allow non-Google library versions

691b8ec

cv: lint camera orchestrator

534de34

Samir-Rashid force-pushed the feat/cv-orchestrator branch from c9a77e6 to 534de34 Compare January 15, 2024 04:10

Samir-Rashid approved these changes Jan 15, 2024

View reviewed changes

dn820 added 2 commits February 7, 2024 08:39

added a localization algorithm

7bc51f8

added terrain height

8094ac2

atar13 marked this pull request as draft February 15, 2024 21:35

Tyler-Lentz added 10 commits February 17, 2024 00:57

update ubuntu version 🙏 (#78)

f227506

Chore/update ubuntu version (#79)

6280e08

* update ubuntu version 🙏 * fix groupadd command

Chore/update ubuntu version (#80)

a43cb42

* update ubuntu version 🙏 * fix groupadd command * fix error

Chore/update ubuntu version (#81)

a7c98dd

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo

Chore/update ubuntu version (#82)

53b0368

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN?

Chore/update ubuntu version (#83)

e0d4fbd

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3

Chore/update ubuntu version (#84)

3320b54

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id

Chore/update ubuntu version (#85)

0d11022

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id * i am at my wits end 🔫

Chore/update ubuntu version (#86)

a5ba8e9

* update ubuntu version 🙏 * fix groupadd command * fix error * fix typo * WHY THE FUCK DID IT NOT MERGE THIS IN? * install cpplint through apt instead of pip3 * try to fix user id * i am at my wits end 🔫 * fix dumbass merge conflict

shijiew555 and others added 8 commits February 29, 2024 16:15

finished integrating segmentation

0d479ae

Merge feat/cv-orchestrator into cv-segmentation-integration. Add mode…

8a4bebe

…l folder and test image folder

Merge remote-tracking branch 'origin/feat/cv-orchestrator' into cv-se…

14a8e3d

…gmentation-integration Added models folder and test images folder.

complete integration test cv_segmentation.cpp

80a3bff

fix lint

74b2c9f

fix torchscript error

a2c143b

Merge pull request #123 from tritonuas/cv-segmentation-integration

d80d340

CV segmentation integration

Merge remote-tracking branch 'origin/main' into feat/cv-orchestrator

e3e663a

atar13 marked this pull request as ready for review March 29, 2024 05:53

Tyler-Lentz requested changes Mar 31, 2024

View reviewed changes

atar13 changed the title ~~Skeleton CV pipeline and SearchState tick~~ CV Pipeline Integration Mar 31, 2024

atar13 added 2 commits March 30, 2024 19:09

add imagemagick to cv_pipeline target

0415e87

move cv_pipeline to use BottleDropIndex

07304fc

atar13 added 7 commits April 1, 2024 22:39

all targets compile

57c47fd

move to use log statements

f7f8b55

also added some comments for some of the cv test files

links to model/image drive files

9031648

also removed some hardcoded model paths and moved them to constructors

Merge remote-tracking branch 'origin/main' into feat/cv-orchestrator

b24ddcf

forgot )

c2c2a26

satisfy cpplint gods

48b7f41

temporarily removing localization test assert 😳

ab5be02

we need to debug the ECEF localization algorithm but it's out of the scope of the feat/cv-orchestrator PR and I want a green checkmark

atar13 requested a review from Tyler-Lentz April 1, 2024 23:38

atar13 added 4 commits April 2, 2024 05:55

fix model downloading make targets

0a6ca2a

automate pulling matching test images

e124a4c

also improved error messages when model can't be loaded

remove extra comment block

7bb880e

satisfy cpplint gods again

b2ba112

Tyler-Lentz approved these changes Apr 2, 2024

View reviewed changes

comments about pulling matching/segmentation models

5a8ee3e

atar13 merged commit 8e07698 into main Apr 2, 2024
2 checks passed

atar13 deleted the feat/cv-orchestrator branch April 2, 2024 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CV Pipeline Integration #73

CV Pipeline Integration #73

atar13 commented Jan 5, 2024

atar13 commented Jan 5, 2024

Samir-Rashid commented Jan 15, 2024

Samir-Rashid left a comment

Samir-Rashid Jan 15, 2024

atar13 Jan 17, 2024

Samir-Rashid Jan 15, 2024

atar13 Jan 17, 2024 •

edited

Loading

Samir-Rashid Jan 19, 2024

Samir-Rashid Jan 15, 2024

atar13 Jan 17, 2024

Samir-Rashid Jan 15, 2024

Samir-Rashid Jan 15, 2024

Samir-Rashid Jan 15, 2024

Samir-Rashid Jan 15, 2024

Samir-Rashid Jan 15, 2024

atar13 commented Jan 17, 2024

Tyler-Lentz left a comment •

edited

Loading

atar13 commented Mar 31, 2024 •

edited

Loading

Tyler-Lentz left a comment

CV Pipeline Integration #73

CV Pipeline Integration #73

Conversation

atar13 commented Jan 5, 2024

Changes

Testing

Feedback

atar13 commented Jan 5, 2024

Samir-Rashid commented Jan 15, 2024

Samir-Rashid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atar13 Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atar13 commented Jan 17, 2024

Tyler-Lentz left a comment • edited Loading

Choose a reason for hiding this comment

atar13 commented Mar 31, 2024 • edited Loading

Tyler-Lentz left a comment

Choose a reason for hiding this comment

atar13 Jan 17, 2024 •

edited

Loading

Tyler-Lentz left a comment •

edited

Loading

atar13 commented Mar 31, 2024 •

edited

Loading