Skip to content

segments: Add missing API for "dot" control on big 7-segments #41

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

Merged
merged 3 commits into from
Jan 14, 2020

Conversation

vdehors
Copy link
Contributor

@vdehors vdehors commented Oct 21, 2019

There are 4 side LEDs around the 4 7-segments on 1.2" packages of HT16k33
(https://www.adafruit.com/product/1270). This commit adds an API to control
them (set and get).

There already was setter and getter to control "ampm" dot (the one at the
top-right). This API is still working but now use the new added functions.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this change!

In general we avoid get/set methods because they can be implemented with properties (like ampm) or data descriptors like colon. ampm and colon already cover two of the four cases you are adding support here.

I'd recommend adding two more properties for top_left_dot and bottom_left_dot that are implemented like ampm. You can use your new get/set methods internally by adding an _ to the start of the name. The other way do do it is by using a data descriptor class for all three.

@vdehors
Copy link
Contributor Author

vdehors commented Oct 22, 2019

Hi tannewt,

Indeed, I didn't see the Colon class (which implemented what I needed btw). I made your changes but the "colon" property may be enough (with doc) ?

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about two_dots_center. Good otherwise. Thanks!

""" Get side LEDs (dots)
See setindicator() for indexes
"""
bitmask = 1 << (index + 1)
return self._get_buffer(0x04) & bitmask

@property
def two_dots_center(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the colon? There shouldn't be two ways to set it.

@bobdog6861
Copy link

I would like to disable the zero blanking of first and second digits, so that I can display the zeros. I cannot seem to figure out where in the driver it is making that decision. Is it in _put, in ht16k33.py ?

@tannewt
Copy link
Member

tannewt commented Nov 19, 2019

@bobdog6861 unless you are testing with this version, please open a separate issue for your question.

@makermelissa
Copy link
Collaborator

I'm thinking this PR may not be necessary. I'm going to attempt to set up a small code example to light up all segments and dots and will paste it here if I can get it working.

@makermelissa
Copy link
Collaborator

Ok, here's how you fill each thing with the way the library currently is now:

import board
from adafruit_ht16k33 import segments
i2c = board.I2C()
display = segments.BigSeg7x4(i2c)

# Clear the display.
display.fill(0)

# Fill Everything
display.print("8888") # Each Digit. You can also use display[0]
display.colon[0] = True  # Middle Colon
display.colon[1] = True  # Left Colon
display.ampm = True  # Upper Dot

@makermelissa
Copy link
Collaborator

@vdehors, I am going to reopen this. I now understand this adds the ability to add separate control of the left side dots, which is excellent. However, as @tannewt suggested, we probably should remove the two_dots_center setter and getters because of the redundancy.

@makermelissa makermelissa reopened this Dec 31, 2019
@kattni
Copy link
Contributor

kattni commented Jan 8, 2020

@makermelissa Thank you for reviewing this PR! Please consider making the necessary changes yourself so we can get this merged. If you need assistance with that, let me know. Thanks!

There are 4 side LEDs around the 4 7-segments on 1.2" packages of HT16k33
(https://www.adafruit.com/product/1270). This commit adds an API to control
them (set and get).

There already was setter and getter to control "ampm" dot (the one at the
top-right). This API is still working but now use the new added functions.
The "colon" property already allow to control these LEDs so remove
two_dots_center because of the redundancy.
Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you!

@makermelissa makermelissa merged commit f836604 into adafruit:master Jan 14, 2020
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 15, 2020
Updating https://github.com/adafruit/Adafruit_CircuitPython_HT16K33 to 2.6.0 from 2.3.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#47 from makermelissa/marquee
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#49 from makermelissa/matrix_scroll
  > update pylint examples directive
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#41 from vdehors/master
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#46 from adafruit/dherrada-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM6DS to 2.0.0 from 1.0.0:
  > Update README.rst
  > Update README.rst
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM6DS#2 from adafruit/lsm6ds_refactor
  > update pylint examples directive
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM6DS#1 from adafruit/dherrada-patch-1
  > added additional rates & sensor identifier

Updating https://github.com/adafruit/Adafruit_CircuitPython_MLX90640 to 1.0.2 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_MLX90640#7 from dglaude/patch-1
  > Merge pull request adafruit/Adafruit_CircuitPython_MLX90640#8 from caternuson/iss2
  > Merge pull request adafruit/Adafruit_CircuitPython_MLX90640#5 from dglaude/patch-1
  > Merge pull request adafruit/Adafruit_CircuitPython_MLX90640#4 from adafruit/dherrada-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_FeatherWing to 1.9.4 from 1.9.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_FeatherWing#51 from makermelissa/master
  > update pylint examples directive
  > Merge pull request adafruit/Adafruit_CircuitPython_FeatherWing#50 from adafruit/dherrada-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_MotorKit to 1.3.3 from 1.3.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MotorKit#24 from caternuson/iss23

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_LSM6DS
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.

5 participants