[ntcore] Add cached topic property

[KangarooKoala]

Should fix #5220
I'm fairly new to NT, so I might've missed some things. For example, NetworkTable and NetworkTableEntry have SetPersistent, ClearPersistent, and IsPersistent methods in C++, but do not have analogs for retained or Java, so I didn't add any for valueTransient.
Additionally, I'm unsure about the naming, particularly about it being two words. However, I didn't want to use just "transient", since I wanted it to be clear that this related only to the value, not the topic's existence as a whole. (Then again, I'm not sure how a topic as a whole would be transient, so perhaps just transient would work) The descriptions also don't (to me, at least) indicate when/why making a topic transient would be wanted, and they can also be completely reworked if desired.

[PeterJohnson]

We kept the persistent methods on those classes only for backwards compatibility, the expectation is that users who want more advanced features will use the Topic class instead.

Regarding updates to the spec, I'm working on a v4.1 spec, so we can plan on integrating this change there instead of directly modifying the v4.0 spec.

[KangarooKoala]

So, what should I do with the changes to the spec? Leave them in the 4.0 spec until the 4.1 spec is added and then transfer them there?

[PeterJohnson]

Re: spec, I've made spec updates in #5659 that I think this should merge cleanly on top of once that goes in.

I'm a bit on the fence about the naming, slightly leaning towards just "transient". The argument for "valueTransient" is that the retained and persistent flags effectively affect both the topic's existence and the storage of the last value. All topics are otherwise transient by default (when the last publisher goes away they vanish). But the user view of retained and persistent is probably more value-oriented: do I want the last value to be retained by the server when all publishers go away? Do I want the last value to be persistent through the server reboots? In that interpretation, the fact the topic exists or not is a little immaterial, and as such, just using "transient" would be more consistent: do I want the last value to be provided by the server at all?

Would a different name be better? E.g. "cached" (default to true) instead of "transient" (default to false)?

[KangarooKoala]

I like cached a lot more than transient, though having the default flags be non-zero would be error-prone. I'll work on it when I have time and see how bad it gets. (Hopefully I'll be able to do tests to verify that it works in different situations)

Also, should I make this only for 4.1+ or also for 4.0?

[PeterJohnson]

It can be for both versions (I'm not sure how it could be version-specific, as the server can have a mix of clients of different versions). Default-true properties feel fine to me, if it's not present in the json, that's simply treated as true.

[KangarooKoala]

Some notes:

• Feedback on wording is still welcome!
• Given the line comment on LocalStorage::TopicData::flags, I'm not sure if changing that to non-zero will affect NT3. Tests passed (locally at least, hopefully CI does too), but I don't know how extensive the test coverage is for NT3.
• I also noticed that GetTopicValueTransient had returned true instead of default-constructing false like GetTopicPersistent and GetTopicCached, so I changed that to match the others, but I'm not 100% sure if that makes sense, given that cached is default true. (But since to my understanding that path is only taken in exceptional circumstances, I think false is the correct value to return)

[KangarooKoala]

Could someone please remove the blocked status? This had been blocked on the creation of NT v4.1, but now that it exists, this isn't blocked anymore.

[PeterJohnson]

When setting the cached property to false, we need to clear the last value, otherwise it will keep sending a stale value.

[PeterJohnson]

Should this affect client behavior as well (e.g. saving the last value in StorageImpl)?

[KangarooKoala]

I don't know why clang-tidy is failing... The error messages (see below for an example) makes it seem like something's wrong with the file resolution, which is weird since I didn't touch anything relating to that, plus I'm not sure why the filename would be empty when it does know which file it's processing. https://stackoverflow.com/questions/63632193/error-unable-to-handle-compilation-expected-exactly-one-compiler-job-in-cl seems similar, but I haven't had time yet to see if there's something similar to what the solution was.

Processing /__w/allwpilib/allwpilib/ntcore/src/main/native/cpp/ntcore_c.cpp
  with ClangTidy
== clang-tidy /__w/allwpilib/allwpilib/ntcore/src/main/native/cpp/ntcore_c.cpp ==
error: no input files [clang-diagnostic-error]
error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]

[KangarooKoala]

Should this affect client behavior as well (e.g. saving the last value in StorageImpl)?

I don't know, but my guess would be that we don't need to add saving the last value in other places, since I can't think of any situations where that value could be used without being stale (outside of the already existing uses). This is the first stuff I've done with NT, though, so I could be wrong. (I also can't think of any other things that should change about client behavior, but that doesn't mean there aren't any.)
Also, what's StorageImpl? I couldn't find it with a grep except for some methods in glass.

[PeterJohnson]

Sorry, I meant to say LocalStorage.

[KangarooKoala]

There is already a lastValue (and lastValueNetwork) field in TopicData- Was there a particular piece of functionality you were thinking of?

[PeterJohnson]

What I’m thinking is whether we should also disable (clear/never set) lastValue in LocalStorage when cached is set to false. What this would mean though is that a normal get() call would never return a value; the only way to get value updates would be the queuing methods (readQueue).

Even if we don’t make that change, we definitely want to make sure clients on connect don’t send the last value to the server for non-cached values.

[KangarooKoala]

Conceptually it would be nicer if there was just no storage at all of non-cached topics, though I don't know enough about how those topics would be used to know whether make them event-only would be problematic. I'll work on the clients sending values on connection issue, though.

[PeterJohnson]

That's what I think too, although it means that users will have to know (e.g. we will need to document) that normal polling get() calls won't work for those topics and they will need to use readQueue() to poll for only new changes.