Blinkenlights

[densminger]

Blinkenlights feature, as discussed in issue #766

[jabdoa2]

I like it. Some small nits and some thoughts about sync.

[jabdoa2]

There are some magic methods on DeviceMonitor to notify it about changes.

[densminger]

I tried to find the magic methods you mentioned but I don't see any on DeviceMonitor aside from __init__ and __call__. I know __setattr__ is used to notify subscribers when a variable is changed, but how can I notifiy subscribers without creating a fake variable like this?

[jabdoa2]

Guess we are good to land this. Some small coding style changes to fix and we can merge this:

Messages

========

mpf/config_players/blinkenlight_player.py

  Line: 2

    pylint: unused-import / Unused deepcopy imported from copy

  Line: 4

    pylint: unused-import / Unused RGBColor imported from mpf.core.rgb_color

  Line: 5

    pylint: unused-import / Unused Util imported from mpf.core.utility_functions

  Line: 29

    pylint: protected-access / Access to a protected member _restart of a client class (col 12)

  Line: 31

    pylint: no-self-use / Method could be a function (col 4)

  Line: 36

    pylint: no-self-use / Method could be a function (col 4)

  Line: 41

    pylint: no-self-use / Method could be a function (col 4)

mpf/devices/blinkenlight.py

  Line: 3

    pylint: unused-import / Unused partial imported from functools

  Line: 7

    pylint: unused-import / Unused ColorException imported from mpf.core.rgb_color

  Line: 13

    pep8: E302 / expected 2 blank lines, found 1 (col 1)

    pep8: E302 / expected 2 blank lines, found 1 (col 1)

  Line: 30

    pep257: D102 / Missing docstring in public method

  Line: 33

    pylint: attribute-defined-outside-init / Attribute '_color_duration' defined outside __init__ (col 8)

  Line: 34

    pylint: attribute-defined-outside-init / Attribute '_cycle_duration' defined outside __init__ (col 8)

  Line: 35

    pep8: E501 / line too long (150 > 120 characters) (col 121)

  Line: 36

    pep8: E501 / line too long (126 > 120 characters) (col 121)

  Line: 38

    pyflakes: F821 / undefined name 'NoReturn' (col 66)

  Line: 42

    pylint: missing-function-docstring / Missing function or method docstring (col 4)

    pep257: D102 / Missing docstring in public method

  Line: 43

    pep8: E501 / line too long (197 > 120 characters) (col 121)

  Line: 47

    pep8: N803 / argument name 'newValue' should be lowercase (col 27)

    pep8: N803 / argument name 'newValue' should be lowercase (col 27)

    pylint: invalid-name / Argument name "newValue" doesn't conform to '^[a-z_][a-z0-9]*((_[a-z0-9]+)*)?$' pattern (col 4)

  Line: 54

    pylint: missing-function-docstring / Missing function or method docstring (col 4)

    pep257: D102 / Missing docstring in public method

  Line: 58

    pylint: missing-function-docstring / Missing function or method docstring (col 4)

    pep257: D102 / Missing docstring in public method

  Line: 63

    pep8: E303 / too many blank lines (2) (col 5)

    pep8: E303 / too many blank lines (2) (col 5)

  Line: 72

    pylint: missing-function-docstring / Missing function or method docstring (col 4)

    pep257: D102 / Missing docstring in public method

  Line: 79

    pylint: missing-function-docstring / Missing function or method docstring (col 4)

    pep257: D102 / Missing docstring in public method

  Line: 84

    pylint: missing-function-docstring / Missing function or method docstring (col 4)

    pep257: D102 / Missing docstring in public method

[atummons]

Looking good. Well I haven't had time to look at mine any further, and you have already solved the problems, so I'll bow out here on fixes. I think it is looking great with Jan's comments there.

Jan, how do we pull this down to run against our local machine? I'd love to pull it down and test it out with Monitor to see how its works. Or do we have to have it merged before that can be done?

[jabdoa2]

Jan, how do we pull this down to run against our local machine? I'd love to pull it down and test it out with Monitor to see how its works. Or do we have to have it merged before that can be done?

I roughly explained this in my video on the IDE setup. Short:

• git clone https://github.com/densminger/mpf.git (clone Daves repo)
• cd mpf (change into new dir)
• git checkout blinkenlights (change to daves branch)
• pip3 install -e . (install in dev mode)

There are other ways with multiple remotes in one checkout but those are a bit tricky to use unless you are very familiar with the inner workings of git.

[atummons]

I roughly explained this in my video on the IDE setup. Short:

* git clone https://github.com/densminger/mpf.git (clone Daves repo)

* cd mpf (change into new dir)

* git checkout blinkenlights (change to daves branch)

* pip3 install -e . (install in dev mode)

There are other ways with multiple remotes in one checkout but those are a bit tricky to use unless you are very familiar with the inner workings of git.

Haven't watched that one yet (in my saved for later on YouTube), and I love the content!!! I will check it out to make sure I understand the ins and outs. But basically same as getting my local, just swapping the steps for Dave, and then do mine to switch back. Makes perfect sense!

[densminger]

So here's an example of some of the "weirdness" that might happen:

Let's say the blinkenlight has cycle_duration=1s (that is, each cycle lasts 1 second, regardless of how many colors the blinkenlight has). The blinkenlight only has one color (red). In this case, the blinkenlight's cycle would look like this:

|       1s      |       1s      |       1s      |       1s      |
|-------|-------|-------|-------|-------|-------|-------|-------|
|  red  |  off  |  red  |  off  |  red  |  off  |  red  |  off  |

If green color is added to the blinkenlight (this assumes off_when_multiple=false), the cycle would look like this:

|       1s      |       1s      |       1s      |       1s      |
|-------|-------|-------|-------|-------|-------|-------|-------|
|  red  | green |  red  | green |  red  | green |  red  | green |

Then a third color (blue) is added:

|       1s      |       1s      |       1s      |       1s      |
|  r |  g  |  b |  r |  g  |  b |  r |  g  |  b |  r |  g  |  b |

Note that each color now is only 1/3 of a second long, since there are three of them per cycle now.

Now, let's say blue is removed from the blinkenlight, while the blinkenlight is currently showing a blue color:

|       1s      |       1s      |       1s      |       1s      |
|  r |  g  |  b |  r |  g  | b|g|  red  | green |  red  | green |
        blue is removed here ^

Notice how blue is displayed when the color is removed, and the light immediately switches to green, since green should be displayed at that point in time now that the blinkenlight only has 2 colors. So the end result is green "flashes" very briefly before red is displayed again and the red/green cycle starts.

I hope this makes sense. It's predictable, and I think if I was playing the game and saw this happen I would understand what happened and why the color change occurred like that.

[jabdoa2]

So here's an example of some of the "weirdness" that might happen:

Let's say the blinkenlight has cycle_duration=1s (that is, each cycle lasts 1 second, regardless of how many colors the blinkenlight has). The blinkenlight only has one color (red). In this case, the blinkenlight's cycle would look like this:

|       1s      |       1s      |       1s      |       1s      |
|-------|-------|-------|-------|-------|-------|-------|-------|
|  red  |  off  |  red  |  off  |  red  |  off  |  red  |  off  |

If green color is added to the blinkenlight (this assumes off_when_multiple=false), the cycle would look like this:

|       1s      |       1s      |       1s      |       1s      |
|-------|-------|-------|-------|-------|-------|-------|-------|
|  red  | green |  red  | green |  red  | green |  red  | green |

Then a third color (blue) is added:

|       1s      |       1s      |       1s      |       1s      |
|  r |  g  |  b |  r |  g  |  b |  r |  g  |  b |  r |  g  |  b |

Note that each color now is only 1/3 of a second long, since there are three of them per cycle now.

Now, let's say blue is removed from the blinkenlight, while the blinkenlight is currently showing a blue color:

|       1s      |       1s      |       1s      |       1s      |
|  r |  g  |  b |  r |  g  | b|g|  red  | green |  red  | green |
        blue is removed here ^

Notice how blue is displayed when the color is removed, and the light immediately switches to green, since green should be displayed at that point in time now that the blinkenlight only has 2 colors. So the end result is green "flashes" very briefly before red is displayed again and the red/green cycle starts.

I hope this makes sense. It's predictable, and I think if I was playing the game and saw this happen I would understand what happened and why the color change occurred like that.

Agreed. It definitely works this way :-). I guess we should add the ordering by priority in blinkenlights_player and with that we should be good to go.

[densminger]

I think this is ready to go.

[atummons]

@densminger Looks like it needs a couple of cleanup items to pass the Travis CI build test. All looks like general formatting and consistency stuff. Once that's cleaned up, it should pass all three tests and get the green check mark. Great work!

mpf/config_players/blinkenlight_player.py
  Line: 28
    pylint: missing-function-docstring / Missing function or method docstring (col 4)
    pep257: D102 / Missing docstring in public method
  Line: 34
    pylint: missing-function-docstring / Missing function or method docstring (col 4)
    pep257: D102 / Missing docstring in public method
  Line: 40
    pylint: missing-function-docstring / Missing function or method docstring (col 4)
    pep257: D102 / Missing docstring in public method

mpf/devices/blinkenlight.py
  Line: 15
    pylint: ungrouped-imports / Imports from package typing are not grouped (col 4)
  Line: 45
    pep8: E502 / the backslash is redundant between brackets (col 50)
    pep8: E502 / the backslash is redundant between brackets (col 50)
  Line: 53
    pep257: D401 / First line should be in imperative mood; try rephrasing (found 'Number')
  Line: 65
    pep257: D401 / First line should be in imperative mood; try rephrasing (found 'The')
  Line: 97
    pep257: D300 / Use """triple double quotes""" (found "-quotes)
  Line: 104
    pep257: D401 / First line should be in imperative mood (perhaps 'Remove', not 'Removes')

[densminger]

Green check!

[atummons]

Sweet!!!

[densminger]

I've been playing with this, and I have a question. Right now, a blinkenlight is a SystemWideDevice object. As such, it exists at a machine level, and the blinkenlight_player has no concept of a game or player state. So, when a mode adds a color to a blinkenlight, and the player's ball drains, that blinkenlight will continue to flash unless the mode specifically tells it to remove its colors when the mode ends (and it would have to "remember" which colors were active when it restarts). Same thing if the game ends. My question is, is this ok, or should I change the blinkenlight to be a LogicBlock instead so it can use the persist_state functionality of logic blocks? It's not exactly a logic block since it interfaces with hardware (lights) so I don't know if this is a good fit or not.

Edit: I just realized that the ConfigPlayer class has a mode_stop method. I'm guessing I could override that method and unload the blinkenlight when modes end. I would just need to keep track of which mode added which colors. So there's likely enough functionality already in ConfigPlayer to handle this correctly.

Edit2: I've marked this PR as draft. I want to get this working before it gets merged.

[atummons]

I was looking at the same thing when I saw your edit. Agreed, just need to use that to cancel when mode ends there and it clears the stack for it's called keys.

As for starting back up, is there a scenario where it would need to do that? I would assume my shot would be in a state that it's profile calls a specific show. So if the shot is in that state when the mode starts again, it should automatically be calling the show, and adding it back to the stack.

[densminger]

Good point. I think it's ok to not remember state. I have it working so that when mode ends it removes the mode's colors automatically. I changed the removeall action to remove_all (to make it consistent with other actions in mpf that use underscores) and I added another action called remove_mode that will remove all the colors added by the calling mode (just in case you want to remove all colors at a point other than when they're automatically removed at mode stop).

Thanks for your input, it helps a lot!

I'll push these changes shortly.

[jabdoa2]

Edit: I just realized that the ConfigPlayer class has a mode_stop method. I'm guessing I could override that method and unload the blinkenlight when modes end. I would just need to keep track of which mode added which colors. So there's likely enough functionality already in ConfigPlayer to handle this correctly.

We already got a way to handle this. It is called "clear_context“. That is called when modes are unloaded, shows are stopped and in any other case. You can have a look at LightPlayer. It is similar to your approach but covers aboslutely everything in MPF. Also you probably do need the change in Blinkenlight with that.

[densminger]

Got it. Thanks, Jan. I changed it to use clear_context instead. So blinkenlight colors are removed when the associated mode/context ends. Example, mode1 adds blue and green, mode2 adds red. When mode2 ends, red will automatically be removed from the blinkenlight, and when mode1 ends, blue and green will be removed. However, a caveat to this is, the colors are also removed when a show ends. So, if a show uses blinkenlights: to add a color to a blinkenlight, then the color will only flash while the show is active, and as soon as that show ends, the blinkenlight's colors will be removed. I am guessing this is ok, just needs to be documented? Let the user know that if they want a blinkenlight's color to remain active for a mode, the mode should add the color to the blinkenlight via a blinkenlight_player and not a through a show. Speaking of which, I'm working on the documentation as well.

I also renamed the key parameter in blinkenlight_player to label instead, because I think that's clearer to the end-user as to its purpose. Finally, I added an express config for blinkenlight_player which does the add action with a uuid/random label.

[atummons]

This needs changed to private _remove_color_with_label (or if it needs to be public, line 92 needs changed to remove the underscore.

[atummons]

Blinkenlight_player calls this, so it looks like the call on 92 needs to be global.

[densminger]

Good catch. That would have been a runtime error. Made me realize I didn't have a unit test for this. I've fixed it and added a test. Will push in next update.

[atummons]

This should be changed to: color: single|str|white
This will allow tokens to be passed in, in addition to color names of color codes.

[jabdoa2]

No. This would change to single|color_or_token|white

[jabdoa2]

Looks like you are right. This does not yet exist. I guess we should create it (as we have it for floats or ints). That will move str parsing out of runtime in most cases.

[atummons]

Can't publish changes, but here's what they are.

Update 347 to: color: single|color_or_token|ffffff.

Then in config_validator:
Add line 81: "color_or_token": self._validate_type_or_token(self._validate_type_color),
Line 669: Swap above the two lines of comments (#) for the following:
def _validate_type_color(self, item, validation_failure_info, param=None):
assert not param
if item is not None:
try:
if isinstance(item, tuple):
if len(item) != 3:
self.validation_error(item, validation_failure_info, "Color needs three components")
return item
except ValueError:
self.validation_error(item, validation_failure_info, "Cannot convert value to color.", 11)

[densminger]

What's the purpose of param=None and assert not param? These aren't in my version of config_validator and I'm not sure the purpose. I'm not sure what's going on here in general.

[atummons]

Jan would have to answer the specifics for you here. But we switched to allowing tokens, and it runs through the token validated to see is it a token on a color, in this case. If it's a token, we know that, so we stop before the validation breaks. If it's not a token, we go through. Without this argument it breaks here as it expects three arguments, but gets 4 due to the token validation.

This is based on the other token_or_xx methods here.

[densminger]

Ah ok. I'll take a look when I get home and push the changes later tonight.

[densminger]

@atummons Ok, both of your fixes have been pushed. I changed the _validate_type_color method a little from what you suggested since it didn't require everything you said. I added tests for both the bug/typo you found, as well as the tokens, so these things should continue to work in the future.

[densminger]

Renamed label back to key as per Jan's suggestion. @atummons, this will mean you'll need to change your code that you're using for testing as well, just fyi.

[atummons]

Awesome work! I'll get latest on your repo and continue to play around with my test machine.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[densminger]

Looks great! Everything works on my end. It's ready to go as far as I'm concerned.

Edit: So it looks like the "stop/remove" express config option will only remove colors that were added without a key (that is, colors that were added with the express config method, or colors that were added without specifying a key). This makes sense, I just need to update the documentation to reflect that.

[jabdoa2]

Looks great! Everything works on my end. It's ready to go as far as I'm concerned.

Thanks for testing and confirming. This has been an awesome collaboration! Thanks @densminger @atummons .

Edit: So it looks like the "stop/remove" express config option will only remove colors that were added without a key (that is, colors that were added with the express config method, or colors that were added without specifying a key). This makes sense, I just need to update the documentation to reflect that.

Yeah exactly. My theory is that 99% of the users will have maximum one color per mode.