Skip to content
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

feature: driver + example for HT16K33 LED driver #341

Merged
merged 7 commits into from Jun 26, 2022

Conversation

chudsaviet
Copy link
Contributor

Holtek HT16K33

Manufacturer link: https://www.holtek.com/productdetail/-/vg/HT16K33
Datasheet: https://www.holtek.com/documents/10179/116711/HT16K33v120.pdf

Mainly, I created it to drive Adafruit 7-segment display - https://www.adafruit.com/product/1270
I'm intending to drive a more high-level library to write to the display.
Actually, the driver is compatible with all HT16K33-based devices.
HT16K33 also have keyscan feature, but they are not yet supported in the driver.

@chudsaviet chudsaviet changed the title feature: driver + example for ht16k33 LED driver feature: driver + example for HT16K33 LED driver Jun 18, 2022
@UncleRus
Copy link
Owner

UncleRus commented Jun 18, 2022

🎉 Now we talking! :)
I have no comments on the structure of the code - it is good and I like it.

But we need a couple of minor corrections regarding the code style.

  1. Indent = 4 spaces
  2. Enum value names must be in uppercase and prefixed with the module name (because they are in the global namespace), something like HT16K33_F0_HZ.
  3. It looks like esp_check.h cannot be included in older versions of ESP-IDF (see CI logs)
  4. The metadata is incorrect, but I'll fix it myself now.

CI error:

/home/runner/work/esp-idf-lib/esp-idf-lib/components/ht16k33/ht16k33.c:20:10: fatal error: esp_check.h: No such file or directory
 #include <esp_check.h>
          ^~~~~~~~~~~~~
compilation terminated.

@chudsaviet
Copy link
Contributor Author

Thank you a lot for fixing metadata!
I tried to do this myself after the CI test failed, but its some pain to install and use latest Ruby on MacOS.
About 4 spaces, do you use any automatic formatter, like clang-format?
As in documentation of this project, I formatted it using clang-format, but v14 instead of v11, because v11 is not readily available.
Do you have any config file to make clang-format comply with this project conventions?

@UncleRus
Copy link
Owner

UncleRus commented Jun 18, 2022

Do you have any config file to make clang-format comply with this project conventions?

Just use

clang-format --style=webkit ht16k33.c

, this will be enough.

I have checked the ESP-IDF versions and yes, ESP_RETURN_ON_ERROR macro is defined only in > 4.4.0. So it will be necessary in source somewhere near the CHECK_ARG() definition:

#ifndef ESP_RETURN_ON_ERROR
#define ESP_RETURN_ON_ERROR(x, log_tag, format, ...) do {                                       \
        esp_err_t err_rc_ = (x);                                                                \
        if (unlikely(err_rc_ != ESP_OK)) {                                                      \
            ESP_LOGE(log_tag, "%s(%d): " format, __FUNCTION__, __LINE__, ##__VA_ARGS__);        \
            return err_rc_;                                                                     \
        }                                                                                       \
    } while(0)
#endif

@trombik
Copy link
Collaborator

trombik commented Jun 18, 2022

Thank you a lot for fixing metadata! I tried to do this myself after the CI test failed, but its some pain to install and use latest Ruby on MacOS.

how difficult is it? homebrew does not work well?

not confirmed but the ruby scripts should run on any supported ruby version (3.x might not work, but should be easy to fix).

@trombik trombik added the enhancement New feature or request label Jun 18, 2022
@chudsaviet
Copy link
Contributor Author

@trombik , the problem with Ruby and MacOS is that MacOS have Ruby 2.x embedded.
After installing Ruby 3.x from Himebrew, you have to either:

  1. Make PATH to favor Homebrew’s Ruby instead of OSes.
  2. Or find Homebrew Ruby and call binaries by full path. I finally did that, but @UncleRus fixed the metadata for me already :)

Still, figuring out the options took some time.
Maybe a Docker container will do a better job? It will be able to have stable Ruby environment.

@trombik
Copy link
Collaborator

trombik commented Jun 19, 2022

@trombik , the problem with Ruby and MacOS is that MacOS have Ruby 2.x embedded. After installing Ruby 3.x from Himebrew, you have to either:

1. Make PATH to favor Homebrew’s Ruby instead of OSes.

2. Or find Homebrew Ruby and call binaries by full path. I finally did that, but @UncleRus fixed the metadata for me already :)

i'm reasonably sure that my ruby scripts should work with 2.x. what if you use the system's ruby? does not work?

Still, figuring out the options took some time. Maybe a Docker container will do a better job? It will be able to have stable Ruby environment.

i wrote them with portability in mind. i migrated to ruby 3.x now, but the time when I wrote, i was on ruby 2.7. i'm not sure what version apple is using, but it should work unless the ruby is so ancient, like 2.0.

@chudsaviet
Copy link
Contributor Author

@trombik , I see. The problem was the “Gemfile.lock”. It contains Ruby version and prevents usage of earlier Ruby versions. Is it even needed in the repo? Looks like a candidate for “.gitignore”.

@trombik
Copy link
Collaborator

trombik commented Jun 21, 2022

@trombik , I see. The problem was the “Gemfile.lock”. It contains Ruby version and prevents usage of earlier Ruby versions. Is it even needed in the repo? Looks like a candidate for “.gitignore”.

Gemfile.lock in the repository is the whole point of using bundler to ensure the results are same across environments.

what if you set path?

bundle config set path ~/.bundle/vendor

this tells bundler to install gems under that path. after that, do:

bundle install

ruby 2.7 works in my test environment.

> ruby --version 
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [amd64-freebsd13]
> bundler --version
Bundler version 2.2.19
> bundle exec rake -C devtools -T
rake readme   # Update README.md
rake rspec    # Run rspec
rake rubocop  # Run rubocop
rake test     # Run all tests

@chudsaviet
Copy link
Contributor Author

@UncleRus , I’m working on “esp_check.h” problem, you don’t have to spend your time in case you were going to :) I see you have done multiple fixes already, thank you a lot !

@UncleRus
Copy link
Owner

It's OK. I just got a little time and decided to spend it usefully :)
Anyway, this PR is ready to be merged, if you don't mind.

@chudsaviet
Copy link
Contributor Author

Ok, I'm not jealous.
Lets merge.

@UncleRus UncleRus merged commit 86fb80d into UncleRus:master Jun 26, 2022
@UncleRus
Copy link
Owner

Thank you!

@hayschan
Copy link
Contributor

HT16K33 also have keyscan feature, but they are not yet supported in the driver.

Any plans for the key scan feature?

@chudsaviet
Copy link
Contributor Author

No, I don't have such plans. But you are welcome to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants