DigitalShoestringSolutions / PowerMonitoring

GNU General Public License v3.0
0 stars 1 forks source link

bugfix/adc-config #30

Closed tobyaharris closed 1 week ago

tobyaharris commented 2 weeks ago

The purpose of this PR is to restore user-friendly support for both BC Robotics and Grove ADC hats to Power Monitoring.

The basic sensing config files - pm_b_1p.toml, pm_b_3pb.toml, pm_b_3pu.toml - should be similar. Some have missed updates.

Fixes #14 , #15 , #29 Work TBD, still draft.

tobyaharris commented 2 weeks ago

Realistically we must accept that this sensing service module is specialised to measuring current, and with basic clamps there will only be a short list of supported hardware (just BCR and Grove). Conveniently one is SPI and the other I2C - so the bus addresses could be hardcoded. This isn't ideal, but I don't see how else to hotfix it within this sensing framework.

tobyaharris commented 2 weeks ago

Drilling into the config, here are the two blocks we have used previously for either BCR or grove adcs.

[device.adc_0]
    module="adc_grove"
    class="PiHat"
    interface="i2c0"
[device.adc_0.config]
    adc_channel=3
[device.adc_0.variables]
    v_in = "v_amp_out"
[device.adc_0]
    module="adc_MCP300X"
    class="MCP3008"
    interface="spi0"
[device.adc_0.config]
    adc_channel=0
[device.adc_0.variables]
    v_in = "v_amp_out"

Specifically I don't understand the classes:

    class="PiHat"
    class="MCP3008"

They are both PiHats (ish, neither are compliant with the proper HAT+ standards). While the BCR uses the MCP3008 ADC, the Grove uses a STM32F030F4P6.

If we were trying to be completely generic, we would supply the bus number, i2c address / spi chip select. But that wouldn't be enough - at some level there still need to be instructions of how to use that bus device to extract data.

I think the only realistic answer is to do what PowerMonitoringBasic does and have these whole blocks selected by string matching.

tobyaharris commented 2 weeks ago

@Greg-Hawkridge I've made what I believe are the minimum changes to user_config to allow selection of either the BC robotics or grove adcs on single or 3 phase. The result is an ugly long config file - please advise on how we can do this better.

tobyaharris commented 2 weeks ago

Testing a single phase build of this fails with:

current-sensing-1      | INFO:main.measure:+---Initialising Modules
current-sensing-1      | Process sensing_dc:
current-sensing-1      | Traceback (most recent call last):
current-sensing-1      |   File "/usr/local/lib/python3.9/multiprocessing/process.py", line 315, in _bootstrap
current-sensing-1      |     self.run()
current-sensing-1      |   File "/app/measure.py", line 79, in run
current-sensing-1      |     asyncio.run(self.async_loop())
current-sensing-1      |   File "/usr/local/lib/python3.9/asyncio/runners.py", line 44, in run
current-sensing-1      |     return loop.run_until_complete(main)
current-sensing-1      |   File "/usr/local/lib/python3.9/asyncio/base_events.py", line 647, in run_until_complete
current-sensing-1      |     return future.result()
current-sensing-1      |   File "/app/measure.py", line 87, in async_loop
current-sensing-1      |     self.initialise_pipelines()
current-sensing-1      |   File "/app/measure.py", line 186, in initialise_pipelines
current-sensing-1      |     pipeline.initialise(self.calculations)
current-sensing-1      |   File "/app/core/pipeline.py", line 14, in initialise
current-sensing-1      |     self.contents.append(calculation_modules[entry])
current-sensing-1      | KeyError: 'power'

which is of course a result of 070ad77eaca52278cbd3b573e803156edbaaff96. As per #29 this needs to be reworked, I'm not reverting to having assumed voltage hardcoded into the depths of nonuser config.

tobyaharris commented 1 week ago

Dropped in favour of #31.