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

Throttle error message on inconsistent encoder timestamps #2869

Merged

Conversation

PeterBowman
Copy link
Member

As a follow-up to #2862, this patch throttles the error message printed whenever an inconsistency in encoder timestamps is detected. The "Encoder timestamps are not consistent! Data will not be published." is logged every few milliseconds and bloats the terminal. I'm using an arbitrary throttle of one second to mitigate this.

@randaz81 I think the nws_ros{.h,.cpp} files at src/devices/ControlBoardWrapper have been accidentally left out after #2845. Perhaps you want to delete them first, then I can rebase this patch accordingly. As a side note, the consistency check patch from #2841 was applied to both nws_yarp and nws_ros in the ControlBoardWrapper directory, but the latter was copied into a different location using a version prior to said changes.

As commented in #2862, I'm still considering other ways to treat (or perhaps disable) consistency checking due to my particular use case.

@PeterBowman PeterBowman temporarily deployed to code-analysis July 12, 2022 15:46 Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis July 12, 2022 15:46 Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis July 12, 2022 15:46 Inactive
@PeterBowman PeterBowman force-pushed the throttle_consistency_checker branch from 3ef44e7 to a4238f6 Compare September 13, 2022 09:51
@PeterBowman PeterBowman temporarily deployed to code-analysis September 13, 2022 09:53 Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis September 13, 2022 09:53 Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis September 13, 2022 09:53 Inactive
@PeterBowman PeterBowman force-pushed the throttle_consistency_checker branch from a4238f6 to 41e406e Compare November 28, 2022 15:38
@PeterBowman PeterBowman temporarily deployed to code-analysis November 28, 2022 15:40 Inactive
@PeterBowman PeterBowman force-pushed the throttle_consistency_checker branch from 41e406e to e125067 Compare January 17, 2023 17:40
@PeterBowman PeterBowman temporarily deployed to code-analysis January 17, 2023 17:41 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis January 17, 2023 17:42 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis January 17, 2023 17:42 — with GitHub Actions Inactive
@PeterBowman
Copy link
Member Author

PeterBowman commented Jan 17, 2023

I'm leaving the changes to the ROS device out of this patch since the device itself has been moved to a different repository at 48a7dd0.

As commented in #2862, I'm still considering other ways to treat (or perhaps disable) consistency checking due to my particular use case.

The use case being: some (but not all) wrapped subjoints stop responding and may resume communication with the board at some later point, in which case and in the current state of things the wrapper device is temporarily muted (not publishing state) by the consistency checker. I would like to keep it alive also in this scenario. Note #2862 allowed unmuting it if all subdevices were made available again. Would it be acceptable in a future PR if the board keeped sending state referring to the still working subdevices, while discarding the outdated timestamps in the average value (used as port envelope)?

Edit: I will look into this with more detail and start an issue.

Edit 2: see #2939.

@PeterBowman PeterBowman force-pushed the throttle_consistency_checker branch from e125067 to f014caf Compare January 31, 2023 18:53
@PeterBowman PeterBowman temporarily deployed to code-analysis January 31, 2023 18:55 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis January 31, 2023 18:55 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis January 31, 2023 18:55 — with GitHub Actions Inactive
@PeterBowman PeterBowman force-pushed the throttle_consistency_checker branch from f014caf to 8fc328c Compare May 24, 2023 11:18
@PeterBowman PeterBowman temporarily deployed to code-analysis May 24, 2023 11:20 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis May 24, 2023 11:20 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis May 24, 2023 11:20 — with GitHub Actions Inactive
@PeterBowman PeterBowman force-pushed the throttle_consistency_checker branch from 8fc328c to bf590cb Compare August 12, 2023 09:06
@CLAassistant
Copy link

CLAassistant commented Aug 12, 2023

CLA assistant check
All committers have signed the CLA.

@PeterBowman PeterBowman changed the base branch from yarp-3.7 to yarp-3.8 August 12, 2023 09:07
@PeterBowman
Copy link
Member Author

I have changed the base branch to yarp-3.8 (it looks like it has triggered some unwanted actions, sorry!). I have also added logging component identifiers (yCIError(name, id(), ...)) to reflect which wrapper has issued the error among many available. Perhaps those IDs could be extended to more places and other devices as well?

@randaz81 randaz81 force-pushed the throttle_consistency_checker branch from bf590cb to db24011 Compare September 19, 2023 08:28
@randaz81 randaz81 temporarily deployed to code-analysis September 19, 2023 09:19 — with GitHub Actions Inactive
@randaz81 randaz81 force-pushed the throttle_consistency_checker branch from db24011 to aad6c24 Compare November 20, 2023 19:27
@randaz81 randaz81 merged commit cdd617e into robotology:yarp-3.8 Nov 20, 2023
48 of 54 checks passed
@PeterBowman PeterBowman deleted the throttle_consistency_checker branch November 20, 2023 21:38
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.

3 participants