Skip to content

Conversation

@tfarago
Copy link
Contributor

@tfarago tfarago commented Jan 19, 2024

How about we simplify it?

@tfarago tfarago requested a review from MarcusZuber January 19, 2024 11:32
@tfarago
Copy link
Contributor Author

tfarago commented Jan 19, 2024

@sarkarchandan after we agree on this addon I will convert it to local and remote versions.

@codecov
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (5ba30fa) 88.38% compared to head (81bf374) 88.37%.

Files Patch % Lines
concert/experiments/addons.py 71.42% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage   88.38%   88.37%   -0.01%     
==========================================
  Files         121      121              
  Lines        8318     8311       -7     
==========================================
- Hits         7352     7345       -7     
  Misses        966      966              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tfarago
Copy link
Contributor Author

tfarago commented Jan 22, 2024

How about this as an alternative: uca-net will send the metadata anyway, so we could add something like grab_with_metadata to libuca (e.g. Phantom will need this anyway since the timestamp is not a part of the image), then ucad can use the actual value and then concert side does not need to decode anything. This way how metadata is handled is unified from the concert side and the actual camera implementations need to implement the interface. This way, also the timestamp checking addon can disappear actually completely and the correct ordering can be just checked by the writer itself. This also removes one complication in the decentralized concert: if I wanted a separate stream for the timestamp checking addon, it would be a waste of network resources, so, no go. If I wanted to check it let's say alongside writer, I would need two addons one one computer, which is not implemented and also shouldn't be implemented because it will circle back to the original problem -> one python process + many consumers = performance decrease. @MarcusZuber @sarkarchandan @gabs1234 we should discuss this.

@MarcusZuber
Copy link
Member

I like it.

If we then also add a trigger_with_metadata, where I can send a dictionary from concert to ucad which will be added to the image metadata also the logging which needs to be synchronized with the actual frames can be handled by this.

@MarcusZuber
Copy link
Member

MarcusZuber commented Jan 22, 2024

I can only have one addon per node?

Can't I just launch multiple addon tango servers as separate processes?

@tfarago
Copy link
Contributor Author

tfarago commented Jan 22, 2024

I can only have one addon per node?

So far yes, but it's not difficult to add more if needed.

@tfarago
Copy link
Contributor Author

tfarago commented Jan 22, 2024

I like it.

If we then also add a trigger_with_metadata, where I can send a dictionary from concert to ucad which will be added to the image metadata also the logging which needs to be synchronized with the actual frames can be handled by this.

Hm, not sure what the best way would be here, I think we four should just meet and discuss this, that will be fastest.

@gabs1234
Copy link
Collaborator

This way how metadata is handled is unified from the concert side and the actual camera implementations need to implement the interface.

This is a very sane approach imo: uca is a standard that the camera drivers have to conform to. So defining a common timestamp format that the drivers would have to convert to makes total sense.

But from what I understood the PCO cameras have integrated timestamps, which is not the case of the phantom (sends a block of timestamps over a seperate 1Gb connection). One could therefore define a camera agnostic timestamp structure, or more generally a metadata structure that could hold all sorts of info (cf. @tfarago what we talked about).

@gabs1234
Copy link
Collaborator

gabs1234 commented Jan 22, 2024

This way how metadata is handled is unified from the concert side and the actual camera implementations need to implement the interface.

This is a very sane approach imo: uca is a standard that the camera drivers have to conform to. So defining a common timestamp format that the drivers would have to convert to makes total sense.

But from what I understood the PCO cameras have integrated timestamps, which is not the case of the phantom (sends a block of timestamps over a seperate 1Gb connection). One could therefore define a camera agnostic timestamp structure, or more generally a metadata structure that could hold all sorts of info (cf. @tfarago what we talked about).

Note that it's possible to define a dictionary-like structure in C (even easier with GLib) using a hash map.

@tfarago
Copy link
Contributor Author

tfarago commented Jan 23, 2024

Yes, that's precisely what I had in mind as well for the metadata.

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.

4 participants