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

Bigclown #8133

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Cynerd
Copy link
Contributor

Cynerd commented Feb 5, 2019

Maintainer: me
Compile tested: Turris MOX and Omnia with current master
Run tested: Turris Omnia with current master

Description:
This is minimal support for BigClown on router. Together with Mosquito these three bigclown-* packages allow to collect data and send them to Influxdb hosted somewhere else. They require some additional python modules that are not available so I added them as well.

@Cynerd Cynerd force-pushed the Cynerd:bigclown branch 11 times, most recently from 1d4fdd1 to 636f9e9 Feb 5, 2019


config gateway 'gateway'
option name 'usb-dongle'
option device '/dev/ttyUSB0'

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

this is probably inappropriate

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

Well that else should be there? It is a default configuration. I can comment it out if you want bit still it is a default value that in most cases is going to be correct.

You can see my respond to your next comment to see an idea of what it should be correctly according me.

echo "Generating bigclown-gateway config file in $CONF"

append gateway name 'name:' usb-dongle
# TODO add udev rules and use different default here

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

this TODO doesn't seem relevant for an openwrt system? no udev?

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

Well there is no udev I know but there is hotplug. It should be possible to make link to have something different in default config other then /dev/ttyUSB0.


service_triggers()
{
procd_add_reload_trigger 'bc-gateway-usb-dongle'

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

this doesn't seem stable...

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

Yes you are correct. I missed that when I renamed some stuff from bigclown provided names. I felt like those were too cryptic and in some cases too long.


append gateway name 'name:' usb-dongle
# TODO add udev rules and use different default here
append gateway device 'device:' /dev/ttyUSB0

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

but yes, the defaults seem..... rather particular.

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

It would be better to have default that works only on bigclown usb dongle but I have to first write good hotplug script for that. I will see what I can do.

append mqtt certfile ' certfile:'
append mqtt keyfile ' keyfile:'

procd_open_instance

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

do you perhaps want to have a UCI option that controls enablign/disabling the service? that lets you have sample config values, but it won't try and automatically start doign things that with default configs. Similar to how wireless is disabled by default.

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

I added one.


stop_service() {
service_stop "$PROG"
ps | grep "$PROG" | grep -v grep | sed -e 's/^\s*\([0-9]\+\)\s.*$/\1/' | xargs -r -- kill -9

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

what on earth is your bigclown doing that procd isn't catching it? also, pgrep is in the base install (even if pkill isn't) which, if this monstrosity is necessary, would make it much simpler. most procd managed services can omit the stop_service function entirely.

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

I suspect that it does not want to die on simple sigterm so they also send sigkill. I am going to drop that. It makes no sense. If it does not die the I am going to report that to upstream project because otherwise it is just stupid.

port: 8086
database: node

points:

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

again, I'd suggest that this is not really a suitable default configuration, but could be shipped as an example perhaps.

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

points are petty much sensible. Those are not most of the times changed unless you edit firmware of modules. With default firmware it makes sense to have exactly this settings. Problem is more with connection settings. I commented those values which should prevent service from starting. That way user has to review config and set connection settings to appropriate values.

}

stop_service() {
service_stop "$PROG"

This comment has been minimized.

@karlp

karlp Feb 6, 2019

Contributor

this clause should be completely superfluous, just try dropping the entire function.

This comment has been minimized.

@Cynerd

Cynerd Feb 10, 2019

Author Contributor

Dropped

@Cynerd

This comment has been minimized.

Copy link
Contributor Author

Cynerd commented Feb 10, 2019

Just so you know I haven't written these scripts nor programs. I just took bigclown provided scripts and packaged them. This does not mean that we don't want to have them tweak to look better it just means that I have no idea why some parts are done in such a weird way. I can try to tweak and clean those scripts to see if it breaks. I will get back to you.

@Cynerd

This comment has been minimized.

Copy link
Contributor Author

Cynerd commented Feb 10, 2019

Just small note. I am pushing fixups. When you say that you think it is ok then I am going to squash them (or you can do it).

Edit: Ok I can't because of tests. Omg. Ok I am not going to do fixups.

@Cynerd Cynerd force-pushed the Cynerd:bigclown branch 9 times, most recently from 7a947b9 to 551ea2d Feb 10, 2019

@cotequeiroz

This comment has been minimized.

Copy link
Contributor

cotequeiroz commented Mar 6, 2019

LGTM. Sorry, I got no pic. ;-)

@commodo

This comment has been minimized.

Copy link
Contributor

commodo commented Mar 6, 2019

@cotequeiroz
feel free to take one from https://lgtm.ro/

i keep a collection there;
[ humor can make SW development less insane ]

@Cynerd Cynerd force-pushed the Cynerd:bigclown branch from 6d73f60 to c96035e Mar 7, 2019

@Cynerd

This comment has been minimized.

Copy link
Contributor Author

Cynerd commented Mar 8, 2019

Ping about merge? I don't have push rights so someone with push rights have to merge it and push it. Thank you.

@Cynerd Cynerd force-pushed the Cynerd:bigclown branch from c96035e to 307e663 Mar 10, 2019

@Cynerd

This comment has been minimized.

Copy link
Contributor Author

Cynerd commented Mar 10, 2019

I am sorry for returning to this when everything was checked already but I found out that I missed something. Package python-schema requires in new version new dependency python-contextlib2. I added it. Please could you @commodo review that new package as well?

@commodo
Copy link
Contributor

commodo left a comment

Split this PR

@commodo

This comment has been minimized.

Copy link
Contributor

commodo commented Mar 11, 2019

@Cynerd

I changed my Approve -> Request changes.

I think we are at a point where it's better to split this PR into smaller PRs and reference this one.

My suggestion would be something like:

  1. 1 PR for all the new Python packages you are adding
  2. 1 PR for all the current Python packages you are modifying
  3. 1 PR for Bigclown; i'd recommend splitting each bigclown package into it's own commit

This PR is now 13 commits.
My brain can typically keep-up with ~5-7 commits to review & re-review.
Github's force-pushed links are not always helpful to track differences between force-pushes.
I would feel [and probably others as well] a bit more comfortable in grouping the changes into viewable/manage-able groups of changes.

Cynerd added some commits Feb 4, 2019

python-chardet: add Python3 variant
Signed-off-by: Karel Kočí <karel.koci@nic.cz>
python-simplejson: add Python3 specific package
Package is rewritten to be current type of python package with variants
but for backward compatibility the simplejson package name was left as
is and new Python3 specific package was named correctly as
python3-simplejson.

Signed-off-by: Karel Kočí <cynerd@email.cz>
python-requests: add Python3 version
Signed-off-by: Karel Kočí <cynerd@email.cz>
python-pytz: add Python3 version
Signed-off-by: Karel Kočí <cynerd@email.cz>
click-log: add package
Signed-off-by: Karel Kočí <karel.koci@nic.cz>
python-decorator: add package
Signed-off-by: Karel Kočí <karel.koci@nic.cz>
python-jsonpath-ng: add package
Signed-off-by: Karel Kočí <karel.koci@nic.cz>
python-schema: add package
Signed-off-by: Karel Kočí <karel.koci@nic.cz>
python-contextlib2: add package
Signed-off-by: Karel Kočí <cynerd@email.cz>
python-influxdb: add package
Signed-off-by: Karel Kočí <karel.koci@nic.cz>
python-appdirs: add package
Signed-off-by: Karel Kočí <cynerd@email.cz>
python-paho-mqtt: add package
Signed-off-by: Karel Kočí <cynerd@email.cz>

@Cynerd Cynerd force-pushed the Cynerd:bigclown branch from 307e663 to 83da0f6 Mar 11, 2019

@Cynerd

This comment has been minimized.

Copy link
Contributor Author

Cynerd commented Mar 11, 2019

Ok. I am closing this and i am going to replace it with three subsequent merge requests.

@commodo

This comment has been minimized.

Copy link
Contributor

commodo commented Mar 11, 2019

Ok. I am closing this and i am going to replace it with three subsequent merge requests.

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.