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

Rework owned sample #391

Merged

Conversation

sashacmc
Copy link
Contributor

@sashacmc sashacmc commented Mar 27, 2024

Rework owned sample processing for pull and sub_channel:

  • Add multithreaded collections: fifo_mt and ring_mt
  • Add channel definition macro: _Z_CHANNEL_DEFINE
  • Add closure for z_owned_sample_t: z_owned_closure_owned_sample_t
  • Define z_owned_sample_ring_channel_t and z_owned_sample_fifo_channel_t channels

(NB: channels for queries and replies will be provided in the separate PR)

@eclipse-zenoh-bot
Copy link

@sashacmc If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@sashacmc sashacmc force-pushed the protocol_pull_sample_rework branch from 9159306 to 2bf7891 Compare April 4, 2024 11:15
@sashacmc sashacmc force-pushed the protocol_pull_sample_rework branch from a058ec5 to 0fadf18 Compare April 4, 2024 15:59
@eclipse-zenoh-bot
Copy link

@sashacmc If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@sashacmc sashacmc force-pushed the protocol_pull_sample_rework branch from 443bc85 to afb24bf Compare April 4, 2024 16:22
@sashacmc sashacmc marked this pull request as ready for review April 4, 2024 16:28
@eclipse-zenoh-bot
Copy link

@sashacmc If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

src/collections/fifo_mt.c Outdated Show resolved Hide resolved
src/collections/fifo_mt.c Outdated Show resolved Hide resolved
src/collections/ring_mt.c Outdated Show resolved Hide resolved
src/api/handlers.c Outdated Show resolved Hide resolved
src/api/api.c Outdated Show resolved Hide resolved
}
return res;
// -- Sample
void _z_owned_sample_move(z_owned_sample_t *dst, const z_owned_sample_t *src) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that a copy more than a move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're just copying the pointer, so we're essentially move ownership of the data.

src/collections/fifo_mt.c Outdated Show resolved Hide resolved
src/collections/fifo_mt.c Outdated Show resolved Hide resolved
}

#if Z_FEATURE_MULTI_THREAD == 1
res = zp_mutex_init(&fifo->_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

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

all the zp_* system calls are deprecated, you should use z_mutex & z_condvar instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see them in this branch :(

tests/z_channels_test.c Outdated Show resolved Hide resolved
@sashacmc sashacmc mentioned this pull request Apr 5, 2024
6 tasks
if (res) {
return res;
}
_Z_RETURN_IF_ERR(zp_mutex_init(&fifo->_mutex))
Copy link
Contributor

Choose a reason for hiding this comment

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

Still needs to replace the deprecrated systems calls: https://github.com/eclipse-zenoh/zenoh-pico/blob/main/src/deprecated/platform.c

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized this changes won't be in this branch until #394 is merged. So we either accept as is and modify later or we wait till #394 is merged and rebase on that. Opinions @Mallets?

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to ago ahed with this PR and we then update #394. @jean-roland what do you think?

src/net/memory.c Outdated Show resolved Hide resolved
@Mallets Mallets added the enhancement Things could work better label Apr 5, 2024
@eclipse-zenoh-bot
Copy link

@sashacmc If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@Mallets Mallets merged commit 3559b3e into eclipse-zenoh:protocol_pull Apr 5, 2024
26 of 49 checks passed
Mallets added a commit that referenced this pull request Apr 8, 2024
* Remove ACK message

* Cleanup pull and subscriber

* Remove Put/Del from ResponseBody

* Cleanup flags

* Comment unusued variables in pull examples

* Fix alignment test

* Add ring to collections

* Pull examples. Fake z_owned_sample_t

* Channels initial commit

* Added channel macros

* Rename sample_ring to sample_channel_ring

* Add fifo and lifo collections

* Moved utils to handlers. Added FIFO handlers. Add z_sub_channel.

* Improve pull example

* Fix return in void functions

* Fix prototype

* Fix newline at end of the file

* Fix windows examples target

* Remove unexisting windows z_sub_channel from CMAke

* Remove unexisting unix c99 z_sub_channel from CMAke

* Rework owned sample (#391)

* Owned sample draft

* Continue owned sample implementation

* [skip ci] Channel macro draft

* [skip ci] Fix element copy

* Add fifo channel

* Add channel clean up and some refactoring/fixes

* Syntax fix

* Refactoring, cleanup

* Rename elem copy to move

* Improve naming

* TODO resolving

* Add channel tests

* Apply review feedback

* Add encoding copy

---------

Co-authored-by: Alexander <Alexander@Bushnev.pro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Things could work better
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants