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

[ntcore] setRetained doesn't work #7399

Open
spacey-sooty opened this issue Nov 17, 2024 · 5 comments
Open

[ntcore] setRetained doesn't work #7399

spacey-sooty opened this issue Nov 17, 2024 · 5 comments

Comments

@spacey-sooty
Copy link
Contributor

Describe the bug
When I use the function setRetained on a topic in Java it doesn't work.

To Reproduce
Steps to reproduce the behavior:

  1. Clone https://github.com/spacey-sooty/nt-set-retained-repro
  2. Run in simulation
  3. Check glass to see topic Foo doesn't exist under retainedImage.
    I don't think this is a glass issue because when I disconnect and reconnect in AdvantageScope the values of any subtopics go away.

Expected behavior
The Topic should have the retained property. This would cause glass to display it under the retained section and AdvantageScope to keep it between disconnects.

Desktop (please complete the following information):

  • OS: Ubuntu 24.04
  • WPILib Information:
    Project Version: 2025.1.1-beta-1
    VS Code Version: 1.94.2
    WPILib Extension Version: 2025.1.1-beta-1
    C++ Extension Version: 1.22.9
    Java Extension Version: 1.36.2024092708
    Java Debug Extension Version: 0.58.2024090204
    Java Dependencies Extension Version 0.24.0
    Java Version: 17
    Java Location: /home/jadey/wpilib/2025/jdk
    Vendor Libraries:
    WPILib-New-Commands (1.0.0)
@PeterJohnson
Copy link
Member

The reason for this is the publish() overwrites the properties since the topic had not yet been published. If you move the setretained to after the publish or pass the retained as part of the publish properties parameter, this will work. I agree this is confusing behavior that should be changed (or at least documented). On the server side, property sets to non-existent topics are ignored, but agreed this is confusing in an API sense.

If we are changing this behavior, we need to redefine what the properties parameter to publish() means. Currently what it is defined to be is the “initial” properties for a topic—the properties that are set if the topic does not already exist. We could change it to a “merge” behavior with previously locally set properties only if the topic didn’t already exist? Note this can still react in an unpredictable way if the topic gets published by someone else in between the setRetained() and publish() calls, so the “right” way will still be to set initial properties through the publish() call directly, but it will work in the likely common case. Not sure if that’s better or worse though.

@spacey-sooty
Copy link
Contributor Author

spacey-sooty commented Nov 18, 2024

so the “right” way will still be to set initial properties through the publish() call directly

I'm probably missing something but when I look at PubSubOption I don't see a way to set retained?

@PeterJohnson
Copy link
Member

PeterJohnson commented Nov 18, 2024

It’s not a PubSubOption, it’s a property set via the properties string passed to PublishEx()—so "{\"retained\": true}". We really should have a better way of setting the standard properties here too (ala PubSubOptions) with an overload, rather than just having to pass a raw JSON string.

@spacey-sooty
Copy link
Contributor Author

Ah that explains why I didn't see it... yeah we should add a better way to that I'll make an issue.

@spacey-sooty
Copy link
Contributor Author

Hm interestingly even when I set that property my struct publisher doesn't persist across disconnects in AdvantageScope but I can only reproduce it with long arrays...

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

No branches or pull requests

2 participants