added hypervisor command ctl to dispatcher loop + config chages #51
Conversation
There was a problem hiding this comment.
Right, for bookkeeping it might be nice to add some description of the PR 😉
Looking at the code, it's requirements are at least https://github.com/XENONnT/daqnt/pull/46.
Now, the idea is to do hypervisor.process-control every logic loop of the dispatcher. The code under review here looks generally fine but without https://github.com/XENONnT/daqnt/pull/46 reviewed, it's hard to say. E.g. should there be any interplay between the dispatcher an the hypervisor? E.g. should we check return codes etc? Otherwise the dispatcher logic loop and the process control might start battling one another which is to be avoided.
So, sure, this PR looks fine but might have to wait for PR46.
| "0": "192.168.131.90", | ||
| "1": "192.168.131.91", | ||
| "2": "192.168.131.92", | ||
| "3": "192.168.131.93", | ||
| "4": "192.168.131.94", | ||
| "5": "192.168.131.95" |
There was a problem hiding this comment.
This does not include any of the NV/MV crates? Perhaps it is worth a comment in the config
There was a problem hiding this comment.
Right, for bookkeeping it might be nice to add some description of the PR wink
Totally right. Sorry. I will add descriptions next time. Let me add some for this one now:
The only change for the dispatcher is that it commands the Hypervisor to look for commands in the db and execute them, now. The actual point where this bit is called in the dispatcher's event loop is probably the most critical bit. At the end of the loop, all the changes depending on the current status should have been executed already, that's why mostly Darryl (but I agree, as far as I can tell), think that either the end of the loop or the beginnig (which is more or less the same spot) would be the best place to put it. Do you see a reason why this wouldn't be a good idea?
In the Hypervisor (mentioned daqnt PR) we now want to have access to NIM IPs, also. That's why we extended the config. Darryl wanted a nested dict for that. I hope I found all the places, where we thus require to ask for the VME part of the nested dict, instead of using the entire dict, as before. I also changed the naming to be consistent with what the object holds.
There was a problem hiding this comment.
What kind of comment would you like?
In some nuclear restart option, I think all the crates (so far only VME) are shut down and restarted. So every crate we add here will be affected by this. So that's probably at least one thing we should consider before adding more crates here. Maybe we want this even.
There was a problem hiding this comment.
Having looked a bit into PR46, I think this is a fine place for such an option.
In some nuclear restart option, I think all the crates (so far only VME) are shut down and restarted.
The comment I would add explicitly is that these are all the TPC crates, not all crates.
|
Is there a reason this isn't merged? |
|
This commit goes hand in hand with changes in nodiaq and daqnt. They should be applied at the same time. I guess we want to do some more relevant changes first to make sure that issues are not coming from this PR when we test the other things. |
|
We haven't had a chance to test the full stack yet. |
No description provided.