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

Seven segment display #488

Open
wants to merge 31 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@martinohanlon

martinohanlon commented Oct 18, 2016

Initial PR for seven segment display

Related to issue #485

martinohanlon added some commits Oct 18, 2016

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 18, 2016

Current coverage is 79.50% (diff: 98.31%)

Merging #488 into master will increase coverage by 1.24%

@@             master       #488   diff @@
==========================================
  Files            31         31          
  Lines          7213       7689   +476   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5645       6113   +468   
- Misses         1568       1576     +8   
  Partials          0          0          

Powered by Codecov. Last update cbdd9b6...3e833d4

codecov-io commented Oct 18, 2016

Current coverage is 79.50% (diff: 98.31%)

Merging #488 into master will increase coverage by 1.24%

@@             master       #488   diff @@
==========================================
  Files            31         31          
  Lines          7213       7689   +476   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5645       6113   +468   
- Misses         1568       1576     +8   
  Partials          0          0          

Powered by Codecov. Last update cbdd9b6...3e833d4

Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
@lurch

This comment has been minimized.

Show comment
Hide comment
@lurch

lurch Oct 18, 2016

Contributor

Ping @SrMouraSilva in case he has any comments.

Contributor

lurch commented Oct 18, 2016

Ping @SrMouraSilva in case he has any comments.

Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
@SrMouraSilva

I add my opinion based in #357 issue

Show outdated Hide outdated gpiozero/boards.py
@@ -1185,3 +1185,86 @@ def on(self):
def off(self):
self.value = False
class SevenSegmentDisplay(LEDBoard):

This comment has been minimized.

@SrMouraSilva

SrMouraSilva Oct 18, 2016

Ignore it
if I want to know if it is on or off, how do?

I suggest a status attribute for it

@SrMouraSilva

SrMouraSilva Oct 18, 2016

Ignore it
if I want to know if it is on or off, how do?

I suggest a status attribute for it

This comment has been minimized.

@SrMouraSilva

SrMouraSilva Oct 18, 2016

A custom __repr__ is a good idea

@SrMouraSilva

SrMouraSilva Oct 18, 2016

A custom __repr__ is a good idea

This comment has been minimized.

@lurch

lurch Oct 18, 2016

Contributor

if I want to know if it is on or off, how do?

is_active will tell you if any LEDs are lit https://github.com/RPi-Distro/python-gpiozero/blob/master/gpiozero/devices.py#L357
(unfortunately that's not in the docs yet)

A custom __repr__ is a good idea

What do you suggest? Is the __repr__ provided by CompositeDevice not good enough?

@lurch

lurch Oct 18, 2016

Contributor

if I want to know if it is on or off, how do?

is_active will tell you if any LEDs are lit https://github.com/RPi-Distro/python-gpiozero/blob/master/gpiozero/devices.py#L357
(unfortunately that's not in the docs yet)

A custom __repr__ is a good idea

What do you suggest? Is the __repr__ provided by CompositeDevice not good enough?

This comment has been minimized.

@SrMouraSilva

SrMouraSilva Oct 19, 2016

What do you suggest? Is the __repr__ provided by CompositeDevice not good enough?

I was thinking also display the character represented

@SrMouraSilva

SrMouraSilva Oct 19, 2016

What do you suggest? Is the __repr__ provided by CompositeDevice not good enough?

I was thinking also display the character represented

This comment has been minimized.

@lurch

lurch Oct 19, 2016

Contributor

I was thinking also display the character represented

If that was to be added, wouldn't that be a better fit for __str__ rather than __repr__ ? shrug

@lurch

lurch Oct 19, 2016

Contributor

I was thinking also display the character represented

If that was to be added, wouldn't that be a better fit for __str__ rather than __repr__ ? shrug

Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Oct 19, 2016

Updated PR based on comments above.

martinohanlon commented Oct 19, 2016

Updated PR based on comments above.

initial_value = kwargs.pop('initial_value', False)
if kwargs:
raise TypeError('unexpected keyword argument: %s' % kwargs.popitem()[0])

This comment has been minimized.

@lurch

lurch Oct 19, 2016

Contributor

Might be worth adding a code-comment in here that the LED segments in the _layouts array are listed in the order (a, b, c, d, e, f, g) ?

@lurch

lurch Oct 19, 2016

Contributor

Might be worth adding a code-comment in here that the LED segments in the _layouts array are listed in the order (a, b, c, d, e, f, g) ?

Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
@lurch

This comment has been minimized.

Show comment
Hide comment
@lurch

lurch Oct 19, 2016

Contributor

Yep, looking nice :-)
Could you undo the file-mode changes though? (I think this sometimes happens by accident when you use git on Windows)

Would it be worth adding an allow_override=False parameter to set_char_layout, such that existing characters in _layouts can only be replaced when allow_override is True?

What does @bennuttall think of my suggestion of the decimal_point property-getter always returning False for 7-segments that don't have a decimal point LED?

Contributor

lurch commented Oct 19, 2016

Yep, looking nice :-)
Could you undo the file-mode changes though? (I think this sometimes happens by accident when you use git on Windows)

Would it be worth adding an allow_override=False parameter to set_char_layout, such that existing characters in _layouts can only be replaced when allow_override is True?

What does @bennuttall think of my suggestion of the decimal_point property-getter always returning False for 7-segments that don't have a decimal point LED?

@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Oct 19, 2016

Updated PR based on comments & added docs

martinohanlon commented Oct 19, 2016

Updated PR based on comments & added docs

from gpiozero import SevenSegmentDisplay
seven = SevenSegmentDisplay(1,2,3,4,5,6,7,8,active_high=False)
seven.display("7")

This comment has been minimized.

@lurch

lurch Oct 19, 2016

Contributor

'seven display seven' sounds a bit tautological ;-)
Perhaps seven.display("5")

And GPIO1 is one of the 'reserved' pins, so maybe pins 2-9 would be better? (only a minor point)

@lurch

lurch Oct 19, 2016

Contributor

'seven display seven' sounds a bit tautological ;-)
Perhaps seven.display("5")

And GPIO1 is one of the 'reserved' pins, so maybe pins 2-9 would be better? (only a minor point)

Show outdated Hide outdated gpiozero/boards.py
@lurch

This comment has been minimized.

Show comment
Hide comment
@lurch

lurch Oct 19, 2016

Contributor

Great docs, thanks :-)

Contributor

lurch commented Oct 19, 2016

Great docs, thanks :-)

@SrMouraSilva

Added some commentaries about "commentaries"

Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
Show outdated Hide outdated gpiozero/boards.py
@SrMouraSilva

This comment has been minimized.

Show comment
Hide comment
@SrMouraSilva

SrMouraSilva Oct 20, 2016

It is being planned you perform some test?

SrMouraSilva commented Oct 20, 2016

It is being planned you perform some test?

@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Oct 22, 2016

I've added tests.

Any advice on locally testing tests, so I dont have to commit changes to get the tests to run.

martinohanlon commented Oct 22, 2016

I've added tests.

Any advice on locally testing tests, so I dont have to commit changes to get the tests to run.

@martinohanlon martinohanlon referenced this pull request Oct 22, 2016

Closed

7 seg displays #485

assert seven_seg[4].active_high
assert seven_seg[5].active_high
assert seven_seg[6].active_high
assert seven_seg[7].active_high

This comment has been minimized.

@lurch

lurch Oct 22, 2016

Contributor

I'm not entirely sure if this first bunch of tests is necessary, since I think they're already covered by the CompositeDevice and LEDBoard tests? (although having too many tests is obviously better than having too few!)

@lurch

lurch Oct 22, 2016

Contributor

I'm not entirely sure if this first bunch of tests is necessary, since I think they're already covered by the CompositeDevice and LEDBoard tests? (although having too many tests is obviously better than having too few!)

This comment has been minimized.

@martinohanlon

martinohanlon Oct 22, 2016

My thinking exactly, it provides a level of protection against future change and it's consistent with other tests such as LEDBarGraph.

@martinohanlon

martinohanlon Oct 22, 2016

My thinking exactly, it provides a level of protection against future change and it's consistent with other tests such as LEDBarGraph.

@lurch

This comment has been minimized.

Show comment
Hide comment
@lurch

lurch Oct 22, 2016

Contributor

Great set of tests, thanks Martin! 👍 Probably worth adding a test_seven_segment_display_no_decimal test too?
And could you revert your changes to debian/rules please.
As you've seen, testing the error-cases is just as important as testing the working-cases :)

Any advice on locally testing tests, so I dont have to commit changes to get the tests to run.

You need to setup a virtualenv, activate the virtualenv, install python-gpiozero into the venv with python setup.py install, then run make develop, and then finally you can run make test to run all the unit-tests.
I like to have separate venvs for Python2 and Python3 (with make develop run in each) so that I can check that all my tests still pass on both 'old' and 'new' Python :)
@waveform80 's said that at some point he plans to add a page like http://picamera.readthedocs.io/en/latest/development.html to the GpioZero docs. (and if #450 gets finished and merged, that'll make local-testing heaps easier)

Hmm, I've just had a crazy idea - what about modifying display to something like:

   def display(self, char, brightness=1):
        """
        Display a character on the 7 segment display.

        :param string char:
            A single character to be displayed.

        :param string brightness:
            The brightness to display the character at, if the display was initialised with ``pwm=True``.
        """
        if isinstance(seven_seg[0], LED):
            if brightness != 1:
                raise ValueError('brightness must be 1 with non-PWM LEDs')
        else:
            if not 0 <= brightness <= 1:
                raise ValueError('brightness must be between 0 and 1')
        char = str(char).upper()
        if len(char) > 1:
            raise ValueError('only a single character can be displayed')
        if char not in self._layouts:
            raise ValueError('there is no layout for character - %s' % char)
        layout = self._layouts[char]
        for led in range(7):
            self[led].value = brightness if layout[led] else 0

;-)

Contributor

lurch commented Oct 22, 2016

Great set of tests, thanks Martin! 👍 Probably worth adding a test_seven_segment_display_no_decimal test too?
And could you revert your changes to debian/rules please.
As you've seen, testing the error-cases is just as important as testing the working-cases :)

Any advice on locally testing tests, so I dont have to commit changes to get the tests to run.

You need to setup a virtualenv, activate the virtualenv, install python-gpiozero into the venv with python setup.py install, then run make develop, and then finally you can run make test to run all the unit-tests.
I like to have separate venvs for Python2 and Python3 (with make develop run in each) so that I can check that all my tests still pass on both 'old' and 'new' Python :)
@waveform80 's said that at some point he plans to add a page like http://picamera.readthedocs.io/en/latest/development.html to the GpioZero docs. (and if #450 gets finished and merged, that'll make local-testing heaps easier)

Hmm, I've just had a crazy idea - what about modifying display to something like:

   def display(self, char, brightness=1):
        """
        Display a character on the 7 segment display.

        :param string char:
            A single character to be displayed.

        :param string brightness:
            The brightness to display the character at, if the display was initialised with ``pwm=True``.
        """
        if isinstance(seven_seg[0], LED):
            if brightness != 1:
                raise ValueError('brightness must be 1 with non-PWM LEDs')
        else:
            if not 0 <= brightness <= 1:
                raise ValueError('brightness must be between 0 and 1')
        char = str(char).upper()
        if len(char) > 1:
            raise ValueError('only a single character can be displayed')
        if char not in self._layouts:
            raise ValueError('there is no layout for character - %s' % char)
        layout = self._layouts[char]
        for led in range(7):
            self[led].value = brightness if layout[led] else 0

;-)

assert (seven_seg[0].value and seven_seg[1].value and seven_seg[2].value and seven_seg[3].value and seven_seg[4].value and seven_seg[5].value and seven_seg[6].value)
seven_seg.display_hex(15)
assert (seven_seg[0].value and not seven_seg[1].value and not seven_seg[2].value and not seven_seg[3].value and seven_seg[4].value and seven_seg[5].value and seven_seg[6].value)

This comment has been minimized.

@lurch

lurch Oct 23, 2016

Contributor

I thought I'd already made this comment, but I can't see it, so perhaps it went missing...
(apologies if it comes up as a duplicate)

Perhaps these assert-value lines would be better as e.g.

    assert seven_seg.value[0:6] == (True, False, False, False, True, True, True)

and / or:

    expected = (True, False, False, False, True, True, True)
    for i in range(7):
        assert seven_seg[i].value == expected[i]
@lurch

lurch Oct 23, 2016

Contributor

I thought I'd already made this comment, but I can't see it, so perhaps it went missing...
(apologies if it comes up as a duplicate)

Perhaps these assert-value lines would be better as e.g.

    assert seven_seg.value[0:6] == (True, False, False, False, True, True, True)

and / or:

    expected = (True, False, False, False, True, True, True)
    for i in range(7):
        assert seven_seg[i].value == expected[i]

This comment has been minimized.

@martinohanlon

martinohanlon Oct 23, 2016

could you revert your changes to debian/rules please.

No idea how that change got into the commit - any advice on how reverse it? So far I've managed to just totally mess up my local repo!

@martinohanlon

martinohanlon Oct 23, 2016

could you revert your changes to debian/rules please.

No idea how that change got into the commit - any advice on how reverse it? So far I've managed to just totally mess up my local repo!

This comment has been minimized.

@lurch

lurch Oct 23, 2016

Contributor

You might be able to do something like git checkout <older-commit-id> debian/rules ? And I think when you push that, this PR would then mark it as unchanged again.

@lurch

lurch Oct 23, 2016

Contributor

You might be able to do something like git checkout <older-commit-id> debian/rules ? And I think when you push that, this PR would then mark it as unchanged again.

This comment has been minimized.

@martinohanlon

martinohanlon Oct 23, 2016

Ta - the simplest method is often the easiest...

@martinohanlon

martinohanlon Oct 23, 2016

Ta - the simplest method is often the easiest...

@lurch

This comment has been minimized.

Show comment
Hide comment
@lurch

lurch Oct 23, 2016

Contributor

BTW based on what @waveform80 is doing over in #459 , you might need to modify your tests to not use pins 2 and 3.

Contributor

lurch commented Oct 23, 2016

BTW based on what @waveform80 is doing over in #459 , you might need to modify your tests to not use pins 2 and 3.

martinohanlon added some commits Oct 23, 2016

@lurch

This comment has been minimized.

Show comment
Hide comment
@lurch

lurch Oct 23, 2016

Contributor

The 'bad' tests still use MockPin(2) and MockPin(3) ?

Contributor

lurch commented Oct 23, 2016

The 'bad' tests still use MockPin(2) and MockPin(3) ?

@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Oct 23, 2016

Arggghhh, unsaved changes in the editor. I really need to sort out a local dev environment. Rather than having to commit each time to test.

martinohanlon commented Oct 23, 2016

Arggghhh, unsaved changes in the editor. I really need to sort out a local dev environment. Rather than having to commit each time to test.

@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Nov 7, 2016

Ive added MultiSevenSegmentDisplay to this PR. Additional tests are needed, which I will do soon, but I wanted to get this in.

martinohanlon commented Nov 7, 2016

Ive added MultiSevenSegmentDisplay to this PR. Additional tests are needed, which I will do soon, but I wanted to get this in.

#validate the messages length
if (self.has_decimal_point and len(message) - message.count('.') > len(self.digits)) or (not self.has_decimal_point and len(message) > len(self.digits)):

This comment has been minimized.

@SrMouraSilva

SrMouraSilva Nov 7, 2016

What are the practical effects of putting a dot if the display does not have support?
If 'ignore' or 'error', I suggest changing it to:

text_size = len(message) - message.count('.')
if (text_size > len(self.digits)):
    ...

else

text_size = len(message) - message.count('.') if self.has_decimal_point else len(message)
if (text_size > len(self.digits)):
    ...
@SrMouraSilva

SrMouraSilva Nov 7, 2016

What are the practical effects of putting a dot if the display does not have support?
If 'ignore' or 'error', I suggest changing it to:

text_size = len(message) - message.count('.')
if (text_size > len(self.digits)):
    ...

else

text_size = len(message) - message.count('.') if self.has_decimal_point else len(message)
if (text_size > len(self.digits)):
    ...

This comment has been minimized.

@martinohanlon

martinohanlon Nov 18, 2016

If the display doesnt support a decimal point, it full stop would be considered to be a character in the message. This would generate an error unless the user had created a character layout for "." using set_char_layout.

@martinohanlon

martinohanlon Nov 18, 2016

If the display doesnt support a decimal point, it full stop would be considered to be a character in the message. This would generate an error unless the user had created a character layout for "." using set_char_layout.

@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Nov 9, 2016

@bennuttall @lurch @waveform80 anyone got any thoughts on why my tests are failing for pypy... I am at a loss.

martinohanlon commented Nov 9, 2016

@bennuttall @lurch @waveform80 anyone got any thoughts on why my tests are failing for pypy... I am at a loss.

martinohanlon added some commits Nov 12, 2016

Merge pull request #4 from martinohanlon/master
Modded multi seven seg tests
Merge pull request #5 from martinohanlon/dev
Dev - modded multi seven seg tests
@martinohanlon

This comment has been minimized.

Show comment
Hide comment
@martinohanlon

martinohanlon Nov 12, 2016

Resolved test issues with pypy... Threading - bah.

martinohanlon commented Nov 12, 2016

Resolved test issues with pypy... Threading - bah.

@bennuttall bennuttall added the RFC label Nov 28, 2016

@SrMouraSilva

This comment has been minimized.

Show comment
Hide comment
@SrMouraSilva

SrMouraSilva Mar 14, 2017

Hello. Any news?

SrMouraSilva commented Mar 14, 2017

Hello. Any news?

@bennuttall bennuttall removed this from In progress in Roadmap Mar 28, 2017

@bennuttall bennuttall added triage and removed RFC labels Feb 15, 2018

@waveform80

This comment has been minimized.

Show comment
Hide comment
@waveform80

waveform80 Feb 19, 2018

Member

Love the idea, and most of the PR (especially that there's a layouts dict which is exactly what I'd want to see). Needs a little love before merge though, and I need to ponder whether this is an example of a class where it'd be acceptable to have a string-based "value" (esp. for the multi-segment one). Scheduling for 1.5.

Member

waveform80 commented Feb 19, 2018

Love the idea, and most of the PR (especially that there's a layouts dict which is exactly what I'd want to see). Needs a little love before merge though, and I need to ponder whether this is an example of a class where it'd be acceptable to have a string-based "value" (esp. for the multi-segment one). Scheduling for 1.5.

@waveform80 waveform80 added this to the v1.5 milestone Feb 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment