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

Channel access monitors: draft for discussion. #24

Merged
merged 10 commits into from
Jun 24, 2021
Merged

Channel access monitors: draft for discussion. #24

merged 10 commits into from
Jun 24, 2021

Conversation

willrogers
Copy link
Contributor

See #22.

Plenty to discuss here:

  • It seemed easier to use one object than to recreate each time as previously
  • We need to distinguish updates from the two monitors as we want to get value from the FORMAT_TIME monitor. We could
    • Have two queues: probably neatest but we'd need to respond to updates on both promptly.
    • Distinguish by timestamp outside of the class (as I have done)
    • Distinguish by timestamp inside the class
  • Recreating the formatter each update seems somewhat heavyweight, but if units or enums or precision change then we do want to create a new one

Any thoughts?

@thomascobb
Copy link

It seemed easier to use one object than to recreate each time as previously

I wasn't sure of the lifetime of the object that got returned. If you yield a Channel then hopefully it is serialized immediately, rather than put on a queue somewhere so that its value might have changed by the time it was serialized. I never checked the tartiflette code...

We need to distinguish updates from the two monitors as we want to get value from the FORMAT_TIME monitor

or we could:

@dataclass
class Updater:
    func: Callable
    async def __call__(self, value):
        await q.put(lambda: self.func(value))
value_monitor = camonitor(pv, Updater(channel.update_value), format=FORMAT_TIME)
...
meta_monitor = camonitor(pv, Updater(channel.update_metadata), events=DBE_PROPERTY, format=FORMAT_CTRL)
while True:
    channel_updater = await q.get()
    channel = channel_updater()
    yield channel

Recreating the formatter each update seems somewhat heavyweight, but if units or enums or precision change then we do want to create a new one

Metadata updates are pretty rare though aren't they? They only happen once on old servers, and only on EGU/PREC/HIGH... changes on new servers?

@willrogers
Copy link
Contributor Author

I wasn't sure of the lifetime of the object that got returned. If you yield a Channel then hopefully it is serialized immediately, rather than put on a queue somewhere so that its value might have changed by the time it was serialized. I never checked the tartiflette code...

I wondered if there was some such consideration.

Metadata updates are pretty rare though aren't they? They only happen once on old servers, and only on EGU/PREC/HIGH... changes on new servers?

Good point.

or we could

Yes, something like that.

@willrogers
Copy link
Contributor Author

willrogers commented Jun 22, 2021

How important is it that only the updates are emitted by the generator? My refactor is presenting a complete update structure each time and one of the tests is failing.

It might have been that Tartiflette decided what needed updating, but it does not appear to. The documentation isn't very detailed on this sort of thing.

@thomascobb
Copy link

Ah, I missed that. I designed that in as it seemed pretty crucial for performance that only the fields that changed in a subscription were sent. Otherwise we're doubling the payload of every message, which doesn't seem great...

Now maintain state in one object but yield a new object containing
the relevant updates each time.

Add a single cainfo request to determine whether the channel is
writeable.
@willrogers
Copy link
Contributor Author

Well this has been a journey. I think this is now behaving correctly, but it is quite involved. There are a couple of naming questions, and one test is failing because the first channel update is not a complete structure now.

coniql/caplugin.py Outdated Show resolved Hide resolved
coniql/caplugin.py Outdated Show resolved Hide resolved
coniql/caplugin.py Outdated Show resolved Hide resolved
coniql/caplugin.py Outdated Show resolved Hide resolved
coniql/caplugin.py Outdated Show resolved Hide resolved
Although the value itself should not have changed, the way it is
formatted may be changed by a meta update, so it is simplest to
resend the value.
@willrogers
Copy link
Contributor Author

There is another detail: we need to send a new value if the formatter changes. I already see an example where an enum is dispatched as a number but then a metadata update changes the formatter but does not resend the string value.

We could check if the formatter has changed before sending a value, or we could send a value for each metadata update since we expect these to be infrequent. I've chosen this for now.

@thomascobb
Copy link

We could check if the formatter has changed before sending a value, or we could send a value for each metadata update since we expect these to be infrequent. I've chosen this for now.

That sounds reasonable

@thomascobb
Copy link

Looks good to me, are the tests passing? I must update this repo to github actions too...

@willrogers
Copy link
Contributor Author

I just updated the final test. Yes, we could do with the tests running here.

Last thing: we have a CAChannel class and then a CAChannelUpdate that inherits from Channel. The naming is a bit confused.

@thomascobb
Copy link

Hmm, could we merge the first update and second update together with the cainfo writeable? I'm not a big fan of that initial value update without metadata, so I think it's worth waiting a couple of milliseconds to present data + metadata in the first update

@willrogers
Copy link
Contributor Author

The test relies on update 1 being value and update 2 being meta. This appears to happen, but I am not sure that we can rely on it. If we can, we could wait for the first two updates and create a full structure first.

Otherwise we have to think of a way to pause the first update until both monitors have returned. I think we can assume that if one monitor returns then the other will.

@thomascobb
Copy link

thomascobb commented Jun 24, 2021

I just updated the final test. Yes, we could do with the tests running here.

Last thing: we have a CAChannel class and then a CAChannelUpdate that inherits from Channel. The naming is a bit confused.

How about CAChannel -> CAChannelMaker, update_value -> channel_from_update, CAChannelUpdate -> CAChannel

@thomascobb
Copy link

The test relies on update 1 being value and update 2 being meta. This appears to happen, but I am not sure that we can rely on it. If we can, we could wait for the first two updates and create a full structure first.

Otherwise we have to think of a way to pause the first update until both monitors have returned. I think we can assume that if one monitor returns then the other will.

We could keep squashing updates until we have had both a time_value and meta_value update?

@willrogers
Copy link
Contributor Author

willrogers commented Jun 24, 2021

Dropping updates, presumably. I mean drop the older updates until we have one of each and we are ready to start.

@thomascobb
Copy link

thomascobb commented Jun 24, 2021 via email

@willrogers
Copy link
Contributor Author

OK, done?

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.

2 participants