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

I added support for sending PMS5003 measurements to domoticz #1

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

mstojek
Copy link

@mstojek mstojek commented Aug 7, 2017

Hello I forked your PMS5003 library and added support for sending datat to domoticz. You may want to add my changes to your code.

Regards,
Marcin

PS: My fork can be found at: https://github.com/mstojek/pms5003-logger

@kzyapkov
Copy link
Owner

kzyapkov commented Aug 10, 2017

Hi,

Thanks for doing this, looks like a useful addition.

A few remarks though:

  • images don't belong in source code. User tutorials are better suited for a wiki page, I don't like binaries in source control.
  • your source contains tabs in a few places -- these should be replaced by spaces, as per PEP8
  • the --port option is duplicated. Probably the global option should be renamed to --serial-port or something, to avoid confusion
  • before the merge, I'd rather have your changes squashed to one or a few commits with meaningful messages

Also, the cli interface can be reorganized to cleanly support different reporting targets, but I hadn't thought this through.

@mstojek
Copy link
Author

mstojek commented Aug 16, 2017

Hello,

Please find my comments:

  • images don't belong in source code
    I did some google and learned that images can be added to Wiki's GIT reporsitory. so U removed images from source code

  • your source contains tabs in a few places -- these should be replaced by spaces, as per PEP8
    I hope that now I do not have any spaces

  • the --port option is duplicated. Probably the global option should be renamed to --serial-port or something, to avoid confusion
    I changed --port to --serial-port, --port to --domoticz-port and --ip_addr to --domoticz-ip

  • before the merge, I'd rather have your changes squashed to one or a few commits with meaningful messages
    Please review the code. We will merge once the code will be ready. Comments are welcome.

I see that I have some inconsistent arguments for domoticz mode. I added to domoticz mode arguments called "-m {oneshot,monitor}" to be able to have oneshot or monitor mode when reporting to domoticz. Honestly I would see the arguments as follows when in domoticz mode:
./pmlog.py -p /dev/ttyAMA0 --enable-pin 24 --warmup-time 35 oneshot domoticz -ip 127.0.0.1 -p 8080 --pm_1_idx 13 --pm_25_idx 14 --pm_10_idx 15 --pm_1_percent_idx 16 --pm_25_percent_idx 17 --pm_10_percent_idx 18

While we have now:
./pmlog.py -p /dev/ttyAMA0 --enable-pin 24 --warmup-time 35 domoticz -ip 127.0.0.1 -p 8080 -m oneshot --pm_1_idx 13 --pm_25_idx 14 --pm_10_idx 15 --pm_1_percent_idx 16 --pm_25_percent_idx 17 --pm_10_percent_idx 18

@mstojek mstojek closed this Aug 16, 2017
@mstojek mstojek reopened this Aug 16, 2017
GPIO object does not have pin attribute anymore. Use line instead
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.

2 participants