Counter control event split #1935
Conversation
avanwinkle
left a comment
avanwinkle
left a comment
There was a problem hiding this comment.
This all makes sense, though it's hard for me to understand what the purpose of the previous code was and why logic blocks were explicitly blocked from the normal control events flow.
|
The unknown for me here is that I'm moving logic_block.mode=mode much earlier, which might have unintended side effects if that being assigned has some effect. Also should check to see if it gets cleared ever (or otherwise gets put in an inconsistent state) |
added is what happens early in config processing, where everything is loaded and attached to the object structure loaded is what happens when a mode is enabled and everything it controls needs to be enabled
…inding request counters defined in modes will only bind when their mode runs and counters defined in machine will only bind at initial load time
bosh
force-pushed
the
counter_control_event_split
branch
from
57359e2 to
6ab3c99
Compare
for machine-scoped counters this should be set as an absolute priority value whereas for mode-scoped counters, the add_mode_event_handler will automatically add the mode priority in, so the counter control event value should be used as an offset
|
devices do not exist across multiple modes, so this should not change any behaviors...
bosh
force-pushed
the
counter_control_event_split
branch
from
e8dd481 to
4df7274
Compare
|
@avanwinkle this latest push is ready for review and possible merging :) (commit below adds a test that fails with two different crashes in dev, and shows noop on mode-specific counters with my fix!) |
…not cause counter value changes
|
Some questions I had on the commits: How does priority affect control events? Why is it needed? Is priority a key word that affects firing order? Where was the offset code implemented? Why delete the mode in mode_device.py device_loaded_in_mode? Is doesn't appear to get used directly after this function? Why remove setting self.mode to none in a call to device_removed_from_mode? That seems like proper functionality based on method name. What if the device was attached to a different mode? Is a check needed for self.mode == mode? |
|
All good questions! It might be slightly easier to talk about things in-context though next time, so I definitely recommend using the Files view in the PR and then pressing the green button "Start Review" so that you can comment on several specific lines in the PR and have contextual discussions on each) (dont forget to hit "Submit Review" at the end!)
Event bindings in general should allow priority management -- because control event handlers are bound slightly differently than your usual event binding (say, with a config_player, where you can attach conditional and priority modifiers inline, e.g.: event_player:
my_event{my_condition==1}.1: my_resulting_events) Line 537 in 461294a so all I had to do was provide the param, and the 0 default will be overridden.
You'll find a lot of
This is necessary because I moved the setting of self.mode earlier in the device lifecycle. It used to be set every time the mode started, and then unset when the mode stopped. My changes require that the counter device knows that it is a mode-specific instance much earlier than before, so I changed it to be set at machine load time, which means it is only set once, instead of every mode start. If I left the It's possible that devices can exist in multiple modes, or that devices attached to modes actually do use whether their .mode property is None or is an actual mode when executing some behavior. I haven't ever tried to redefine a device in multiple modes (I'm not sure how I would do that even, I assume that if I define the same instance name of the same device type in two places, the config validator will tell me it's not allowed in all cases (I've definitely seen it do that before with say, two of the same named switch or light). But if it allows it, would only one instance need all the configuration? What if I configured the same device name with two different configurations?) but if you could define a device in multiple modes, wouldn't it need a .modes array, not a single .mode value? My guess is that this is a leftover from an older MPF version, or a feature that never got fully built out. I think it's a pretty reasonable thing that devices defined in modes should always know the mode they are attached to, rather than only knowing it when the mode is running (thus, moving .mode=mode earlier and not clearing it out), but I do think this is the riskiest part of the change, though the tests dont seem to mind :) Edit: but yeah, so if a device could attach to multiple modes, wouldnt the old implementation imply that the device only really knows about the most recently-attached one? I did actually include a self.mode==mode check in one place, though honestly I'm pretty sure it's overkill, as it will only ever be called with a matching mode. |
|
@jimhenshaw aw I was wrong, to start an inline review you go to the files tab, then you hover a changed line, click the blue + icon, type your comment, and then press Start Review there. In the end you submit your review comments all together as a batch with that upper right button. |
3 participants
#1933
I think the code change itself is kinda gross, I'm sure there's a better way (and the commit that moves my_counter.mode=mode earlier seems like it could cause a meaningful difference elsewhere in the code (nothing fails, but it seems like there would have been a reason why mode was attached only at mode-start-time)).
Still, this solution is close enough to a real one that I figured it's worth putting up to get comments on before I go trying to figure out a good test to prove it.