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

Add streaming support for hx711 device #2793

Merged
merged 8 commits into from
Sep 10, 2019
Merged

Add streaming support for hx711 device #2793

merged 8 commits into from
Sep 10, 2019

Conversation

pjsg
Copy link
Member

@pjsg pjsg commented Jun 16, 2019

Fixes #2733 .

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

The current hx711 is deficient in a couple of ways. It only allows reading from channel A with a fixed gain, and it only supports single reads. This PR fixes both of these problems by allowing the full choice of inputs and gains, and also adds a streaming callback interface that returns multiple samples in a buffer. The sample rate is either 10Hz or 80Hz -- and this depends on how the hardware is configured (i.e. it isn't software controllable).

@marcelstoer
Copy link
Member

Ideally we could get the original author @christakahashi to review this but that's probably unlikely to happen.

@christakahashi
Copy link
Contributor

I'm no longer equipped to test this code. I can read it over to check that it appears to work right for the HX711 hardware. If that's helpful let me know.

@marcelstoer
Copy link
Member

I can read it over to check that it appears to work right

Sure, any type of review is certainly welcome.

@marcelstoer marcelstoer removed this from the Next release milestone Jul 26, 2019
@marcelstoer marcelstoer added this to the Next release milestone Sep 10, 2019
@marcelstoer marcelstoer merged commit 32ad759 into nodemcu:dev Sep 10, 2019
@marcelstoer
Copy link
Member

marcelstoer commented Sep 10, 2019

Argh, this failed CI. @pjsg would you mind taking a look?

hx711.c:18:1: error: unknown type name 'task_handle_t'
 static task_handle_t tasknumber;
 ^
hx711.c: In function 'hx711_data_available':
hx711.c:117:5: error: implicit declaration of function 'task_post_medium' [-Werror=implicit-function-declaration]
     task_post_medium(tasknumber, control->active);
     ^
hx711.c: In function 'luaopen_hx711':
hx711.c:332:3: error: implicit declaration of function 'task_get_id' [-Werror=implicit-function-declaration]
   tasknumber = task_get_id(hx711_task);
   ^
cc1: all warnings being treated as errors
../../Makefile:422: recipe for target '.output/eagle/debug/obj/hx711.o' failed
make[2]: *** [.output/eagle/debug/obj/hx711.o] Error 1
make[2]: Leaving directory '/home/travis/build/nodemcu/nodemcu-firmware/app/modules'
../Makefile:380: recipe for target '.subdirs' failed

Source: https://travis-ci.org/nodemcu/nodemcu-firmware/jobs/583107655#L1187

P.S. I have no idea why CI wasn't triggered when you initially submitted the PR.

@TerryE
Copy link
Collaborator

TerryE commented Sep 13, 2019

@marcelstoer @pjsg what happened to the discussion and agreement to hold off committing other PRs post master drop, so that I could do the Lua 5.1 to Lua 5.3 realignment without having to work through other PRs being committed in parallel?

This has been a long hard task for me. What is absolutely essential here is that we can build the firmware and in particular the modules and their supporting libraries against both Lua 5.1 and 5.3 and to do this I've had to remove use of undocumented extensions and some eLua crap such as needing to check explicitly for lightweight function types. I've had to rework all of the modules to get them to conform to the Lua API. And of course applying all of the NodeMCU changes to the Lua 5.3 distro.

In terms of this commit, the least effort for all is now to let the commit stand. Once we've reviewed reviewed these API changes and we are happy with these (so modules changes can be considered stable) then we can start to schedule releasing PRs relating to other module changes.

All I am asking is that the other committers honour this short moratorium. This is a request, but if other committers continue commit across this during this few week period, then it makes it almost impossible to continue with this work. So if you do want to carry on committing then please let me know, and I will regretfully abandon the whole Lua 5.3 upgrade program.

@marcelstoer
Copy link
Member

what happened to the discussion and agreement to hold off committing other PRs post master drop

I am sorry, I didn't remember. I didn't intentionally step on your toes.

In terms of this commit, the least effort for all is now to let the commit stand.

That's where I disagree. I think I should just revert it to bring dev back to a compilable state. That would also stop all the emails I'm getting about broken cloud builder builds 😦

@TerryE
Copy link
Collaborator

TerryE commented Sep 13, 2019

Marcel, do whatever works best. This is a one off that I can cope with. 😊

@pjsg
Copy link
Member Author

pjsg commented Sep 13, 2019

Yes -- I'll fix it up tomorrow. My guess is that some header inclusion stuff changed since I submitted it. It builds fine on my local system (or it did a few months ago)

vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Dec 27, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Dec 27, 2019
marcelstoer added a commit that referenced this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants