Skip to content

Conversation

@ladyada
Copy link
Member

@ladyada ladyada commented Sep 8, 2018

seems to work, will take it outside to the daystar tomorrow

@ladyada ladyada closed this Sep 8, 2018
@ladyada ladyada reopened this Sep 8, 2018
@ladyada ladyada requested review from a team and tannewt September 8, 2018 05:47
@ladyada
Copy link
Member Author

ladyada commented Sep 8, 2018

maybe we want to add
"- pip install -r requirements.txt" to travis.yml in cookiecutter?

@ladyada
Copy link
Member Author

ladyada commented Sep 8, 2018

i give up. code works, will figure out why travis is failing later

@sommersoft
Copy link
Collaborator

@ladyada,

maybe we want to add "- pip install -r requirements.txt" to travis.yml in cookiecutter?

We've had the discussion of adding the Blinka/PyPI stuff to cookiecutter. Just need to narrow down everything that should/can be included and how to handle the TODOs.

code works, will figure out why travis is failing later

Looks like it's failing to build rpi-ws281x:

Compiling ws281x library...

lib/ws2811.c: In function \xe2\x80\x98setup_pwm\xe2\x80\x99:\nlib/ws2811.c:395:23: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
     dma_cb->dest_ad = (uint32_t)&((pwm_t *)PWM_PERIPH_PHYS)->fif1;
                       ^
lib/ws2811.c: In function \xe2\x80\x98setup_pcm\xe2\x80\x99:
lib/ws2811.c:460:23: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
     dma_cb->dest_ad = (uint32_t)&((pcm_t *)PCM_PERIPH_PHYS)->fifo;
                       ^
cc1: all warnings being treated as errors\nmake: *** [lib-built/ws2811.o] Error 1

Looks like the same error reported in rpi-ws281x issue #12.

@ladyada
Copy link
Member Author

ladyada commented Sep 8, 2018

yeah i need to figure out how to not install that on travis, but do install it on raspi only

Copy link
Collaborator

@sommersoft sommersoft 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! A few nit-noid things...

Also, I may change the VEML6070 library to match this. I wrote it without the use of @property and setter structure...

# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.
"""
`Adafruit_VEML6075`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to match the filename's case: `adafruit_veml6075`

docs/conf.py Outdated
# Uncomment the below if you use native CircuitPython modules such as
# digitalio, micropython and busio. List the modules you use. Without it, the
# autodoc module docs will fail to generate with a warning.
# autodoc_mock_imports = ["digitalio", "busio"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will need to uncomment, and mock micropython.

@integration_time.setter
def integration_time(self, val):
"""Set how long the VEML samples data.
Can be 50, 100, 200, 400 or 800ms"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most other libraries only have the docstring in the @property function. RTD may render integration_time twice or combine them, which could be confusing. I would just add something like, "Valid times are: 50, 100, 200, 400, or 800" to the end of the @property docstring.

Although, after reviewing a couple other doc pages, maybe we should start/change the docstrings to include the setter functions. As they stand, the API documentation does not show the available args. This may be a good in-the-weeds topic for the meeting on Monday though. Thoughts @kattni @tannewt?

README.rst Outdated
Usage Example
=============

.. todo:: Add a quick, simple example. It and other examples should live in the examples folder and be included in docs/examples.rst.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Travis will flag this .. todo.

@ladyada
Copy link
Member Author

ladyada commented Sep 8, 2018

@sommersoft great review, thanks - i fixed all the simple things up.

@sommersoft
Copy link
Collaborator

np! you even fixed one .. todo that I missed! 😄

@ladyada
Copy link
Member Author

ladyada commented Sep 9, 2018

whew finally travis passes - take a look! hardware coming soon but this library can be released now :)

@ladyada
Copy link
Member Author

ladyada commented Sep 9, 2018

thanks! mergin, @tannewt - lemme know if you have any changes, we wont be releasing this sensor for a week or two

@ladyada ladyada merged commit eb3b1b9 into adafruit:master Sep 9, 2018
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.

Looks good to me!

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.

3 participants