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

Change callbacks #48

Open
pljakobs opened this issue Sep 25, 2024 · 11 comments
Open

Change callbacks #48

pljakobs opened this issue Sep 25, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@pljakobs
Copy link
Contributor

I've been thinking about callbacks a bit more.

So far, we've discussed callbacks on streaming the data, I think there might be another classe which is callback on change.

The way I understood your suggestion on streaming is that I would be able to modify the data in motion - encrypting or decrypting it. That's cool for masquerading / encrypting fields.

If we had callbacks on change, we could essentially make the whole configuration event driven.

Let's take the example of {"network":{"connection":{}}}

If I could just subscribe to either the whole connection or to sub-items thereof, I could handle the whole reconfiguration in that callback.
I would also be able to take the change set and send it out to all possibly connected frontends (if a user had more than one connected) and other controllers (for global settings such as scenes)

The challenge I see with that is: how to determine the change set? The frontend may send any granularity of data, from the whole database json to a single property, in order to reconfigure the network, though, you'd want to wait until address, netmask and gateway are consistent - which may or may not involve a change to all of them. Maybe that's really for the callback to figure out and do some basic sanity testing on, or alternatively, the front end must take care of this to make sure that it sends the whole connection object as one update

@pljakobs
Copy link
Contributor Author

just noticed that this might be partially or fully overlapping with the discussio we had.
The main point is: callbacks could be valid for both data at rest and data in motion - for different purposes.

@mikee47
Copy link
Owner

mikee47 commented Sep 25, 2024

Yes, thanks for opening this issue, will be addressing all the callbacks in one PR I think.

Regarding onChange callbacks the correct time to call them is after a commit. It would be a requirement for this that the front-end sends all changes together, and grouped together by store as the writer only keeps one store open during streaming updates. In practice I'd have thought this would be fairly easy to accommodate?

These callbacks are probably fairly straightforward to implement as they are very similar to asynchronous updates, and can likely go on the same queue. The only differences would be that the callback will receive a reader instead of an updater, and the callback persists in the queue. We'd also need a way to remove change callbacks from the queue.

@mikee47
Copy link
Owner

mikee47 commented Sep 25, 2024

how to determine the change set?

At present the store just keeps a dirty flag but that just gets set whenever a writeable (non-const) data pointer is fetched, so it's pretty basic and in fact doesn't mean the data has actually changed, it's just a presumption. In practice, I think that's pretty reasonable since we'd expect the front end to only send changes through.

Tracking changed properties within a store would require a memcmp for each property. And tracking those would require some flags, perhaps a bitset for each Store. That's the bit which is going to take some work.

Although this change information is only relevant if there is a registered change callback, so the change flags could be stored in the actual callback instead....

@pljakobs
Copy link
Contributor Author

The change set does not necessarily give the right answer, I think.
If the user changes the ip-address but not the default gateway, the change may be valid with the address alone or may need the gateway to be changed if the network portion has changed, too.

So any change flag can only indicate that a reconfiguration may be necessary, but not if the data set is complete to acutally do the reconfiguration. That would, as you indicated, be the task of the application to make sure that it sends a change set as a complete set, even if it may span multiple stores. Then this could be regarded as a transaction and the callback be triggered after it.

Another way I was thinking about was being able to subscribe to a change mask, basically every property gets a position in a bit field and a specific callback function is called once one or all mask bits are set and then the mask is cleared.

I'm thinking about something like

    cfg.addCallback(ConfigDB::changeMask::ipAddress|ConfigDB::changeMask::Gateway|ConfigDB::changeMask::NetworkMask, ConfigDB::changeMaskType::AND, myFunction);

not sure how difficult that would be to implement and it seems a bit non-C++ to me as it is very dynamic, but maybe it's worth thinking about it?

@mikee47
Copy link
Owner

mikee47 commented Sep 25, 2024

task of the application to make sure that it sends a change set as a complete set, even if it may span multiple stores. Then this could be regarded as a transaction and the callback be triggered after it.

Then we need something else.

In the Basic_Config sample we use a ConfigDB::HttpImportResource to handle updates. It has an onComplete handler which the application can use to perform any post-transaction processing. Since it gets a reference to the ImportStream object, that is probably where we can store a set of change flags for the application to act upon.

Would that work for you?

@pljakobs
Copy link
Contributor Author

that sounds reasonable. The ImportStream is the serialized json, right?

@mikee47
Copy link
Owner

mikee47 commented Sep 25, 2024

It's the stream that de-serialises incoming JSON and writes it to the database.

@pljakobs
Copy link
Contributor Author

I'm not yet sure how I would have to use it. I guess I'll have to see what it looks like.

@mikee47
Copy link
Owner

mikee47 commented Sep 25, 2024

I was forgetting - you just pull the whole thing into RAM and call ConfigDB::Status status = app.cfg->importFromStream(ConfigDB::Json::format, *bodyStream);, which is simpler. I see you then do the checks on SSID, IP, etc. Well, the addition of change flags just simplifies the code you have there as you don't need to note the old values and do a comparison, which I suspect is what you're really after.

So maybe we just stick the change flags in the Status structure. That's a generic structure so the flags would just be a fixed block of RAM big enough to store a bit for every property we could reasonably expect in a database. Maybe 256 bits?

We could then generate types/methods for the AppConfig database to interpret those flag bits, something like this:

using PropFlag = AppConfig::PropertyFlag;
AppConfig::PropertyFlags changes(status);
if (changes[PropFlag::ssid]) {
  ...
}
if(changes[PropFlag::ip]) {
 ...
}

The AppConfig::PropertyFlag would be an enum class definition identifying all the properties in the database, and AppConfig::PropertyFlags would be a BitSet of those.

This is just a rough outline, I'm sure we can come up with something better, using namespaces or similar to match the structure of the database itself more closely. Things like AppConfig::PropertyFlag::Network::Ap::ssid and so on.

So I'm not seeing any real benefit in trying to work this into a callback...

@mikee47 mikee47 added the enhancement New feature or request label Sep 26, 2024
@pljakobs
Copy link
Contributor Author

this looks good - would this track true changes (so as you said, usign strcmp)? Might be a bit intense.

@mikee47
Copy link
Owner

mikee47 commented Sep 27, 2024

this looks good - would this track true changes (so as you said, usign strcmp)?

Yes, that's the idea. String properties are processed and added to the string pool anyway, to get a stringId, so equal strings get the same id. Basically every simple property resolves to 1, 2, 4 or 8 bytes so the added overhead will be pretty minimal.

Might be a bit intense.

I'm being mindful not to over-optimise any of this. If it turns out things are using too much RAM, flash or too slow then we can address those individually.

@mikee47 mikee47 changed the title callbacks - again Change callbacks Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants