NAU7802 implementation and supporting refactor #33
Conversation
|
The first commit is rough but it should take care of a few of the goals listed above.
I ran out of time to start in earnest on the frontend changes, settings migration, and basic unit/integration testing, so those remain left to implement before this PR can be considered for proper review/merge. |
filament_scale_enhanced/__init__.py
Outdated
| tare=8430152, | ||
| reference_unit=-411, | ||
| # chips=["hx711", "nau7802", "none"], | ||
| chip="nau7802", |
There was a problem hiding this comment.
Might be worth keeping the default as the cheaper chip, that tends to be what most implementations seem to use in all the guides. A default with the other type may confuse beginners.
| chip="nau7802", | |
| chip='hx711', |
There was a problem hiding this comment.
Definitely agree there. That default was a debug change that I forgot to change out before commit. 🤦
- Assure that default
chipishx711, notnau7802
filament_scale_enhanced/__init__.py
Outdated
| if chip == "nau7802": | ||
| from .scale.nau7802 import NAU7802 | ||
|
|
||
| bus_id = int(self._settings.get(["nau_bus_id"])) | ||
|
|
||
| self._logger.debug("Initializing NAU7802, bus_id: [{}]".format(bus_id)) | ||
| self.scale = NAU7802(offset, cal_factor, bus_id) | ||
| self._logger.debug("Initialized NAU7802, is_ready: [{}]".format(self.scale.is_ready())) | ||
| elif chip == "hx711": | ||
| from .scale.hx711 import HX711 | ||
|
|
||
| clock_pin = self._settings.get(["hx_clockpin"]) | ||
| data_pin = self._settings.get(["hx_datapin"]) | ||
|
|
||
| self._logger.debug("Initializing HX711, clock_pin: [{}], data_pin: [{}]".format(clock_pin, data_pin)) | ||
| self.scale = HX711(offset, cal_factor, clock_pin, data_pin) | ||
| self._logger.debug("Initialized HX711, is_ready: [{}]".format(self.scale.is_ready())) | ||
| else: | ||
| self.scale = None |
There was a problem hiding this comment.
It'll save some boiler plate and give room for different backends in the future (maybe say an external arduino), if we from . import scale at the top of the file, then use getattr and pass the whole config payload into the library. Let each implementation handle parsing it during instantiation. Along with the logging statements. Potentially a try/except for if the backend configured doesn't exist
| if chip == "nau7802": | |
| from .scale.nau7802 import NAU7802 | |
| bus_id = int(self._settings.get(["nau_bus_id"])) | |
| self._logger.debug("Initializing NAU7802, bus_id: [{}]".format(bus_id)) | |
| self.scale = NAU7802(offset, cal_factor, bus_id) | |
| self._logger.debug("Initialized NAU7802, is_ready: [{}]".format(self.scale.is_ready())) | |
| elif chip == "hx711": | |
| from .scale.hx711 import HX711 | |
| clock_pin = self._settings.get(["hx_clockpin"]) | |
| data_pin = self._settings.get(["hx_datapin"]) | |
| self._logger.debug("Initializing HX711, clock_pin: [{}], data_pin: [{}]".format(clock_pin, data_pin)) | |
| self.scale = HX711(offset, cal_factor, clock_pin, data_pin) | |
| self._logger.debug("Initialized HX711, is_ready: [{}]".format(self.scale.is_ready())) | |
| else: | |
| self.scale = None | |
| self.scale = getattr(scale, chip)(settings=self._settings) |
There was a problem hiding this comment.
That's a great idea and I'm all for simplifying this code.
- Move per-device instantiation details to
scalelibrary - Implement config parsing in
scalelibrary to determine which subclass to init and handle per-subclass variables - Use
getarr()to initself.scalewith the active config using the above implementation duringon_startup()
filament_scale_enhanced/__init__.py
Outdated
| chip = self._settings.get(["chip"]) | ||
| if chip == "hx711": | ||
| self.scale.enable() | ||
|
|
||
| v = self.scale.get_weight() | ||
|
|
||
| self._logger.debug("weight: [{}]".format(v)) | ||
|
|
||
| self._settings.set(["lastknownweight"], v) | ||
| self._settings.save() | ||
|
|
||
| self._plugin_manager.send_plugin_message(self._identifier, v) | ||
| self.hx.power_down() | ||
|
|
||
| if chip == "hx711": | ||
| self.scale.disable() |
There was a problem hiding this comment.
I can't recall why it's necessary to power up/down here for the hx11. But maybe implementing a initial_check_weight method, that by default calls get_weight and is overriden by the hx711 chip might be a touch cleaner
Something like this in the hx11 class
def initial_check_weight(self) -> float:
self.enable()
weight = self.get_weight()
self.disable()
return weightThere was a problem hiding this comment.
This was one of the first parts of the code I dug into and this is an example of just trying to keep things as identical as possible to the original implementation before making more changes.
When I was working on the CircuitPython HX711 GPIO bitbang driver I definitely observed some oddities around 'enable' vs 'disable' state management. In that driver's case I basically just abandoned all hope of tracking a 'powered up' vs 'powered down' state and pared the implementation down to the barest of basics, the raw ADC count read. That has its own downside - leaving the clock line low means the first read-in could provide a stale reading - but its easy enough to just read a few dozen counts, throw away the first read (or just remove outliers) before averaging.
Even with that knowledge though there's always going to be a risk of severe timing issues causing odd behavior in general, which is kinda unavoidable when bitbanging from high in the stack like this. Even with the much more lightweight CircuitPython, platforms like nrf and esp32 (which do BLE/WiFi management in background threads) can see some weird timing issues pop up that can totally wreck the driver's reliability. So this kind of mitigation doesn't surprise me or seem out of place, even if I'm not sure precisely why it was included.
Anyway, long story short, I think this "state toggle" should probably stick around (for now, at least). And I agree that it probably belongs in the scale.hx711 wrapper rather than here in the main plugin code. :)
- Move
hx711state toggle on read out of thecheck_weight()and into thescaledriver wrapper
|
Finally got some time set aside to get my dev environment reconfigured and dig into this again. Should have a few solid, multi-hour chunks of time to dive in over the next few days. Right off the bat I found a few embarassing typos that broke things, which I fixed in e6d858d. These slipped in because my original setup involved some copy/paste from my Pi devbox to my main dev PC (gross!). Now I'm just doing git commits directly from the devbox with my dev PC connecting via the VSCode On the dev system I've got two filament scales attached, one via HX711 on the usual pin 20/21 combo and the other via a NAU7802 on I2C. This way I can easily switch between the two by changing the setting in the Octoprint config. Not sure what the order of operations is going to be but I've got a few chunks of to-do items.
|
|
Latest commit tackles a lot more than I like to bundle into a single commit but most of it had to land at the same time, so I'll let it slide. 😎
I've been doing functional testing as I go, and this commit's had at least these functions manually tested.
This iteration doesn't use Also rearranged a lot of code to group similar things together and satisfy linters. My first commit was a total mess. 😂 These are the linters/formatters I tested with and their basic configs, if different from the project's config files:
If these linter settings are good then I can enshrine most (or all) of them in their respective configs and add them to the GitHub I've used Next stuff... Didn't dive into REST API Auth, JS/templates, settings versioning, or unit test implementation in these changes but I spent a bit of time researching each as I worked through things. Of those, the JS/template work is probably the foggiest in my head at the moment. For the others I have at least some basic implementation ideas/examples in mind. Let me know if you've got any feedback on the latest state of the code, or if I've missed implementing any changes suggested on the first commit. If all goes well I should have another few chunks of time like this in the coming days. |
| if command == "tare": | ||
| self.last_api_time = time.time() |
There was a problem hiding this comment.
A method I often use here is
getattr(self, command)()This makes adding new functionality just adding a method, keeps the entry function smaller, and makes unittesting easier.
As it all involves hardware, unit testing is probably hard. But we might be able to write a mock scale class. But I wouldn't consider that level required to merge.
| if scale_type not in SCALE_TYPES: | ||
| raise ValueError(f"Invalid scale_type: {scale_type}") | ||
|
|
||
| if scale_type == "rpi_hx711": | ||
| from .rpi_hx711 import RPI_HX711 |
There was a problem hiding this comment.
I think I mentioned an importlib method previously. This keeps the logic out of the main entry point which is good.
|
I've also caught Caught covid, so I'll see how functional I am. It's put me away from where my 3D printer is, but I have setup a VPN now, which makes it easier to get to. VS Code + Remote SSH is pretty nifty. I think I produced a dist and uploaded for most of my testing, it wasn't super efficient. An editable package would have been potentially easier. |
|
Oof, hope you're feeling better! I ended up getting sidetracked into other tasks over the last few weeks but I'm hoping to have more time to dive back in next week. |
Turns out Covid doesn't mix well with ADHD! Took months for the brain fog to lift, and I'm just starting to get enough energy back to keep up with work. But I have been 3D printing again! So I'm going to try and get through this, and try and come up with ways to make it easier to approve/merge/release changes. |
Refactor to support the NAU7802 ADC for weight measurements in addition to the HX711. More details can be found in issue #32.
Goals
ScaleinterfaceRelated Open Issues
This will address issue #32 and may also fully or partially address #28, #26, and #18.
Moving to use of a common "scale" interface and settings versioning should also lay the groundwork to allow for addressing #31, #22, and #17.