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

Align examples and remove reading from stdin #359

Merged
merged 9 commits into from
Mar 21, 2024

Conversation

oteffahi
Copy link
Contributor

@oteffahi oteffahi commented Feb 28, 2024

These changes remove reading from stdin for all examples, and align their behaviors and outputs across projects.

This PR is part of an effort across currently active Zenoh projects. To maintain examples' implementations as close to each other as possible, any changes identified in other projects will be replicated in this PR. As such, please do not merge these changes until all PRs are ready for merging.

Related PRs:

@eclipse-zenoh-bot
Copy link

@oteffahi 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.

@eclipse-zenoh-bot
Copy link

@oteffahi 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.

@oteffahi oteffahi marked this pull request as ready for review February 28, 2024 15:41
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Cppcheck (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@oteffahi
Copy link
Contributor Author

oteffahi commented Mar 6, 2024

The CI failed because the c11/z_get example currently does not block to wait for responses; I worked under the assumption that the z_get function is a blocking call that awaits a final queryable response before returning to the main function. I will fix this as soon as possible.

void reply_dropper(void *ctx) {
(void)(ctx);
printf(">> Received query final notification\n");
pthread_cond_signal(&cond);
pthread_cond_destroy(&cond);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the public sync mechanisms found in include/zenoh-pico/system/platform.h, this impacts all the following pthread_cond and pthread_mutex calls, see z_ping for an example of usage.

@@ -112,6 +130,14 @@ int main(int argc, char **argv) {
printf("Unable to send query.\n");
return -1;
}
struct timespec query_timeout;
clock_gettime(CLOCK_REALTIME, &query_timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the public timer mechanisms found in include/zenoh-pico/system/platform.h, this impacts all the following clock calls, see z_ping for an example of usage.

#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <zenoh-pico.h>

#if Z_FEATURE_QUERY == 1
#define QUERY_TIMEOUT 10 // query timeout in seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we'll have to revise this example when timeout is exposed by zenoh pico

@Mallets
Copy link
Member

Mallets commented Mar 15, 2024

@@ -54,7 +55,7 @@ void app_main(void) {

char *buf = (char *)pvPortMalloc(256);
z_clock_t now = z_clock_now();
for (int idx = 0; 1;) {
for (int idx = 0; idx < N;) {
Copy link
Member

Choose a reason for hiding this comment

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

In this case after 10 publications the example will exit, however all other examples kepp publishing indefinitely.

@@ -27,13 +27,17 @@
#endif

#define KEYEXPR "demo/example/**"
#define N 10
Copy link
Member

Choose a reason for hiding this comment

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

We should align the behaviour with all other bindings, I believe we don't wait for just 10 samples by default.

Copy link
Contributor Author

@oteffahi oteffahi Mar 19, 2024

Choose a reason for hiding this comment

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

Other bindings do not implement single-threaded pub/sub examples, therefore we only need to align the configuration of these examples across their different plateform-specific implementations. In this PR, the configuration is as follows:

  • On unix (c11, c99): N must be provided as a CLI argument to run the example. This is also used in our CI to run the test with a known number of messages. N is initialized to int32:max and can be overwritten through CLI arguments.
  • On windows: since getopt is unavailable, N is initialized to int32::max.
  • On freertos: I understand that CLI argument-passing may not be possible on certain embedded targets that run on freertos, therefore N is initialized to 10 to avoid running the example indefinitely. Should we change it to int32::max ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the other comment on this file, I'll update all implementations to start with a default number of samples of int32:max, and allow unix implementations to overwrite it through CLI argument.

Copy link
Member

Choose a reason for hiding this comment

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

The behaviour of a z_pub in all languages is to publish indefinitely, so uint32::MAX is close enough :)

@Mallets Mallets merged commit 7c724c3 into eclipse-zenoh:main Mar 21, 2024
49 checks passed
@jean-roland jean-roland mentioned this pull request Apr 5, 2024
Mallets added a commit that referenced this pull request Apr 5, 2024
* add support for qos settings in sample

* - silence cpp compiler regading compound literals and type conversions,
- add missing dots in docs

* fix _z_qos_t definition and doc

* fix z_qos_t definition and doc

* - replace binary literals (non-core C) with hexadecimal ones
- replace _z_n_qos_unmake with _z_n_qos_unmake_public in subscription.c

* make _z_n_qos_unmake static inline to prevent linking errors

* fix format

* reduce qos size to 1 byte; use getters to extract individual qos settings

* fix _z_n_qos_make to no longer use compund literals to avoid warnings from zenoh-cpp

* fix z_qos_default() signature

* Add Flipper platform (#351)

* Add Flipper platform

* Fix warnings and other platform builds

* move z_n_qos_t related functions into .c (to hide designated initializers from c++)

* z_sample_t docs update to include qos

* move qos functions back to header and remove usage of designated initializers

* format

* fix: Bump sphinx doc build dep to `7.2.6` (#361)

* replac zp_ prefix for platform functions that are also available in zenoh-c

* fix format

* replace zp_ prefix with z_ for zp_random_u64

* put back previously removed zp_ functions as deprecated

* added missing new line at the end of the file

* typo fix

* move deprecated platform header to a separate folder

* Add config endpoint in raweth locator (#364)

* feat: add ethtype to reth endpoint config

* feat: type renaming

* fix: remove unused macro call

* feat: remove static raweth config

* feat: use defines for separators

* feat: add mapping function

* fix: add default raweth mapping

* feat: add debug traces on raweth config  parsing

* fix: ethtype validity test

* fix: strtok parsing calls

* chore: clang-format

* fix: include first mapping entry in lookup

* fix: appease the tyran codacy

* Fix bug reading from ws (#370)

* Expose timeout option in z_get_options_t (#375)

* Expose timeout option in z_get_options_t

* Add default get timeout

* Align examples and remove reading from stdin (#359)

* Remove reading from stdin, align example implementations

* Update examples tests

* Adjust z_sub_thr output

* Fix z_get single query examples

* Replace pthread uses with z_mutex and z_condvar

* Add multi-thread feature condition to z_get examples

* Update error message for features absence

* Update z_get expected output in modularity test

* Update sample count for freertos single-thread examples

* fix: remove unused variable warning (#379)

* Changing references to zenoh:master to zenoh:main. (#381)

* correct unsigned atomic in refcount.h (#382)

* Fix refcount cast (#384)

* Fix esp32 CI (#386)

* fix: const discard warning

* build: fix platformio dependency check

* Serial timeout (#383)

* correct unsigned atomic in refcount.h

* made espidf freertos compatible with timeout on read

---------

Co-authored-by: Luca Cominardi <luca.cominardi@gmail.com>

* Run clang-format (#388)

* Fix misra violations (#390)

* chore: run clang format

* fix: misra issues

* ci: Allow building zenoh from arbitrary branch in build-check workflow (#389)

* ci: Allow building Zenoh from arbitrary branch in build-check workflow

* Update other jobs

---------

Co-authored-by: OlivierHecart <olivier.hecart@adlinktech.com>

* build: point ci to zenoh protocol_changes branch

* fix: check declare <I> flag at declare level

* fix: missing function prototype warning

* build: switch zenoh branch to interests

* fix: ignore unknown final interest

* fix: missing argument in tigger_local_subs

* fix: add wait join step in z_pub for raweth testing

* fix: raweth memory leaks

* feat: add message number option to z_sub

* fix: null malloc when cloning rc list

* doc: remove obsolete types

* fix: encoding option made fragment test fail

* fix: replace all deprecated system calls

* fix: improve raweth test stability

---------

Co-authored-by: Denis Biryukov <denis.biryukov@zettascale.tech>
Co-authored-by: Alexander <sashacmc@gmail.com>
Co-authored-by: Michael Ilyin <milyin@gmail.com>
Co-authored-by: Mahmoud Mazouz <mazouz.mahmoud@outlook.com>
Co-authored-by: OlivierHecart <olivier@zettascale.tech>
Co-authored-by: Alexander <Alexander@Bushnev.pro>
Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com>
Co-authored-by: Geoff Martin <geoff.martin@zettascale.tech>
Co-authored-by: Lieven <lieven.merckx@gmail.com>
Co-authored-by: Luca Cominardi <luca.cominardi@gmail.com>
Co-authored-by: OlivierHecart <olivier.hecart@adlinktech.com>
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.

None yet

4 participants