ser2net: Added support for config file. #5302

TimelessNL · 2017-12-19T20:38:28Z

Maintainer: Jasper Scholte [email protected]
Compile tested: (x86, GEODE, LEDE 17.01.02)
Run tested: (x86, GEODE, LEDE 17.01.02, tested written configuration and startup of ser2net)

Description:
This adds support for ser2net /etc/config/ser2net config file and corresponding init.d/ser2net script.

Note: this changeset is based on https://github.com/JiapengLi/OpenWrt-luci-app-ser2net/blob/master/openwrt-packages-add-luci-app-ser2net-support.patch

TimelessNL · 2017-12-19T20:51:22Z

Seems like the author name from my Github.com account is invalid for some reason. This shouldn't matter I suppose?

pprindeville

The handling of options needs to be more complete and have sane defaults.

pprindeville · 2017-12-19T20:55:47Z

net/ser2net/files/etc/config/ser2net

+        option 'state' 'raw'
+        option 'timeout' '30'
+        option 'device' '/dev/ttyUSB0'
+        option 'options' '9600 1STOPBIT 8DATABITS'


I think 2-stop bits is only ever used with 110 baud, and in any case the stop bits should be inferred from the baud rate if not given. Likewise, 8-databits can be safely inferred unless something else is given.

Lastly, you don't seem to be including the parity (which, when absent, can be omitted and safely default to NONE which is the prevalent assumption in this post-Latin1/UTF8 world where we can't assume that everything is 7-bit ASCII).

I can be mistaken but the default options is now set to 1STOPBIT? So your comment about 2-stop bits is not right?

Currently I put these patches in a pull-request and left the defaults as is. So if something can improved sure, no problem.

I personally don't know good defaults but I saw these: [9600 8DATABITS NONE 1STOPBIT] from here: https://www.domoticz.com/wiki/SharingSerialDevices is that ok?

I can be mistaken but the default options is now set to 1STOPBIT? So your comment about 2-stop bits is not right?

Not sure what you think isn't right about it. I'm saying that 2 stopbits are never used except at 110 bps.

Most low-end serial devices are 38400. Most high-end devices are 115200.

Sorry for not understanding. But where do you see 2 stop bits? Currently the option is "1STOPBIT" that is 1 stop bit right?

I'm saying in the options, only the baud rate should be required (if that). The rest can be implied by the "normal" settings for any given seen (2-stopbits for 110 baud, 1-stopbit for anything else... no parity, 8-databits).

@TimelessNL Not sure what you mean by (1). Are you talking about a GUI in LUA?

(2) sounds good to me.

@karlp I'm not making assumptions. I'm going by the ECMA and ITU standards for RS-232. If someone wants 2-stopbits they can explicitly call that out. I'm just saying that for baud rates above 110, the same default is 1-stopbit.

I don't like the current approach. Please change it to support the following config style:

config proxy ... option baudrate <int> option databits <8|7|6|5> option parity <odd|even|none|space|mark> option stopbits <1|2> list options 'remctl' list options 'local' list options '-xonxoff'

Yes, this requires more work for generating the config since we need a mapping but I really think, this is the way we should go.

I agree with @karlp that we don't add assumptions here.

@pprindeville Aah sorry about (1) I was already thinking ahead. I also got a pull-request for a LuCI-app so that's why I mentioned the dropdown :P. ofc this is invalid here.

@ both. Is it ok if I wait before doing anything until you 2 figured it out?

@mhei Thanks. I will have to look into this coding a little bit. This all is still a little bit new for me. So it could take some time.

The optional 4th argument to config_get is a default value.

pprindeville · 2017-12-19T21:51:43Z

Seems like the author name from my Github.com account is invalid for some reason. This shouldn't matter I suppose?

That's a question for @champtar.

champtar · 2017-12-19T22:07:17Z

@TimelessNL Travis is only looking at the commit, in this case the Author field of the commit
Putting you real name in that, and having a matching signed-off-by is good practice in my opinion
Regards

pprindeville · 2017-12-19T22:17:15Z

@champtar Thanks for that clarification.

TimelessNL · 2017-12-19T22:20:31Z

@champtar Ah I thought I had set this in my git client. Will check, thanks!

karlp · 2017-12-20T16:39:18Z

net/ser2net/files/etc/init.d/ser2net

+	        state=`uci get ser2net.@proxy["$i"].state 2> /dev/null`
+	        timeout=`uci get ser2net.@proxy["$i"].timeout 2> /dev/null`
+	        device=`uci get ser2net.@proxy["$i"].device 2> /dev/null`
+	        options=`uci get ser2net.@proxy["$i"].options 2> /dev/null`


this is pretty gross. Look at how other packages use config_load instead.

Ok will look into that.

I'll review this part when you updated the PR. Please update this PR and do not create a new one.

Just one note: I think about adding the dynamically generated part of the configuration file, i.e. using /etc/ser2net.conf as base and then adding the generated lines add the end. This way, the user can manually edit some defaults or additional items which are not (yet) configurable via UCI but also has the comfort of controlling the main aspects via UCI. What do you think about this idea?

That would be feasible indeed. But I also think that if people miss something in our current package they should notify it so it gets supported for everyone. Otherwise people will do local changes and do not bother to mention them upstream. I know this is a little bit harsh but I think it's for the best.

But how would you know what the devices were?

karlp · 2017-12-20T16:40:36Z

net/ser2net/files/etc/init.d/ser2net

+	        if [ -z "$options" ]; then
+	                echo "$tcpport":"$state":"$timeout":"$device" >> "$CONFIGFILE"
+	        else
+	                echo "$tcpport":"$state":"$timeout":"$device":"$options" >> "$CONFIGFILE"


you know this can be more than just a port right? you can put in things like ipv4,tcp,localhost,2000 in that first field even.

If I check https://linux.die.net/man/8/ser2net and look at the Configuration section they also mention it as TCP port.

Their setup is: TCP port : state : timeout : device : options

That man file is out of date. Look at the docs within the existing /etc/ser2net.conf

Ok, so the new name is "network port". So changing it to "networkport" will be sufficient?
ser2net.conf.txt

karlp · 2017-12-20T16:41:36Z

net/ser2net/files/etc/init.d/ser2net

+	PID=`cat /tmp/ser2net_pid`
+	rm -f /tmp/se2net_pid
+	kill -9 $PID 	
+}


just make this a proper procd service. there's no need for this mucking about trying to track pidfiles manually. it would be simpler to do this properly.

Ok will look into that.

Concur with @karlp.

karlp · 2017-12-20T17:03:30Z

You probably should also have been using a branch for this, so that your rtl433 package didn't just appear here as well :)

TimelessNL · 2017-12-20T18:29:08Z

@karlp, Haha you are to fast, I was working on that but I typed the wrong command :)
Should be fixed now.

TimelessNL · 2017-12-20T19:13:22Z

Ok, I took the init.d files form "cron" and "minidlna" as an example and came up with this new init.d ser2net script. Procd is still new for me but I think I included everything?

pprindeville

Doesn't make sense to not populate device.

pprindeville · 2017-12-20T19:33:22Z

net/ser2net/files/etc/init.d/ser2net

-	        fi
+	config_get state $cfg state
+	config_get timeout $cfg timeout
+	config_get device $cfg device


Shouldn't the device also be requisite?

It probably is :) I'll add this

mhei · 2017-12-20T18:58:58Z

net/ser2net/files/etc/config/ser2net

@@ -0,0 +1,7 @@
+config 'proxy'
+        option 'tcpport' '8000'


I'm not happy with "tcpport", nor the "upstream" version "networkport". I would prefer just simple "port" here. I'm still thinking about whether we should accept the advanced configuration items of upstream, i.e. passing this simply down into the config, or whether we should force this to be a simple port number. The latter facilitates a UCI type check...

mhei · 2017-12-20T19:00:36Z

net/ser2net/files/etc/config/ser2net

@@ -0,0 +1,7 @@
+config 'proxy'


Hm, "proxy" is not my first choice, but in the absence of a better idea... The important thing is, that we might add additional types later, e.g. for LED configuration etc.

Why not just device? one section for each device line?

there is already a parameter device? I think changing it to device makes it confusing. And I suppose ser2net is proxying the serial data to network data. So it seems ok to me?

The port # and the device name are unique strings... I would differentiate the blocks based on either one of those, and make the other one a mandatory parameter.

well, they're not actually. ser2net can have the same port and multiple devices, or multiple ports for the same device, and have them enabled/disabled separately at runtime via the control port.

mhei · 2017-12-20T19:20:18Z

net/ser2net/files/etc/config/ser2net

+        option 'state' 'raw'
+        option 'timeout' '30'
+        option 'device' '/dev/ttyUSB0'
+        option 'options' '9600 1STOPBIT 8DATABITS'


I don't like the current approach. Please change it to support the following config style:

config proxy ... option baudrate <int> option databits <8|7|6|5> option parity <odd|even|none|space|mark> option stopbits <1|2> list options 'remctl' list options 'local' list options '-xonxoff'

Yes, this requires more work for generating the config since we need a mapping but I really think, this is the way we should go.

I agree with @karlp that we don't add assumptions here.

mhei · 2017-12-20T19:25:23Z

net/ser2net/files/etc/init.d/ser2net

+	        state=`uci get ser2net.@proxy["$i"].state 2> /dev/null`
+	        timeout=`uci get ser2net.@proxy["$i"].timeout 2> /dev/null`
+	        device=`uci get ser2net.@proxy["$i"].device 2> /dev/null`
+	        options=`uci get ser2net.@proxy["$i"].options 2> /dev/null`


I'll review this part when you updated the PR. Please update this PR and do not create a new one.

mhei · 2017-12-20T19:27:46Z

net/ser2net/files/etc/config/ser2net

+config 'proxy'
+        option 'tcpport' '8000'
+        option 'state' 'raw'
+        option 'timeout' '30'


Please add proper default handling when create config file in case timeout option is not given in UCI.

mhei · 2017-12-20T19:29:48Z

net/ser2net/files/etc/config/ser2net

@@ -0,0 +1,7 @@
+config 'proxy'
+        option 'tcpport' '8000'
+        option 'state' 'raw'


Yes, I know "upstream" calls this state... but please change it to "protocol".

Why? when I saw protocol in this later, I thought it was an error, mixingin tcp/udp options or v4/v6, both which go in the "port" section of the config string.

mhei · 2017-12-20T19:34:24Z

net/ser2net/files/etc/init.d/ser2net

+	        state=`uci get ser2net.@proxy["$i"].state 2> /dev/null`
+	        timeout=`uci get ser2net.@proxy["$i"].timeout 2> /dev/null`
+	        device=`uci get ser2net.@proxy["$i"].device 2> /dev/null`
+	        options=`uci get ser2net.@proxy["$i"].options 2> /dev/null`


Just one note: I think about adding the dynamically generated part of the configuration file, i.e. using /etc/ser2net.conf as base and then adding the generated lines add the end. This way, the user can manually edit some defaults or additional items which are not (yet) configurable via UCI but also has the comfort of controlling the main aspects via UCI. What do you think about this idea?

mhei · 2017-12-20T19:42:13Z

Seems that my review and your rework were overlapping - sorry if this results in extra work on your side.

TimelessNL · 2017-12-20T19:44:42Z

@mhei no issue. Will rework some more based on your additional comments. Be back shortly.

pprindeville

Procd changes required.

pprindeville · 2017-12-20T19:56:31Z

net/ser2net/files/etc/init.d/ser2net

+	config_load 'ser2net'
+	ser2net_create_config config || return 1
+	procd_open_instance
+	procd_set_param command "$PROG" -c "$CONFIGFILE"


For procd, you will need -n.

pprindeville · 2017-12-20T20:01:54Z

@mhei Don't forget about rtscts flow-control.

So if we don't have defaults, then what do we do when databits or parity or stopbits are omitted?

karlp · 2017-12-20T20:29:47Z

You still need some global config option with an "enable" flag to allow turning off your (it will always be, unfortunately) limited uci support and allow using the native config file.

pprindeville · 2017-12-20T20:38:50Z

You still need some global config option with an "enable" flag to allow turning off your (it will always be, unfortunately) limited uci support and allow using the native config file.

@karlp What would a canned config look like? What serial lines can you count on reliably being present?

TimelessNL · 2017-12-20T20:44:42Z

Sorry but it's a little bit hard to keep-up with all the comments.
Especially since some get hidden when I update some lines.

Would it be an option to comment globally instead inline?

pprindeville · 2017-12-20T20:51:26Z

@mhei @karlp I'm confused. Does enable turn off ser2net globally, or just cause it to process any additional UCI configuration?

pprindeville · 2017-12-20T20:56:35Z

@mhei Am I missing something, or is only a single config block with a single device permitted? I have a Xeon 1U server with a Startech PEX16S952LP 16-port serial PCI card in it, that is both firewall and terminal server for all of my headless servers (which run console=ttyS0,115200n8r).

mhei · 2017-12-20T21:18:47Z

We should definitely allow multiple config proxy sections. Each section will result in one ser2net config line. The enabled option in one section should only prevent this section to be processed, but not to disable ser2net globally. We could add a

config global
    option enabled ...

config proxy
    option ...

to address this.

TimelessNL · 2017-12-20T23:14:29Z

Correct me if I'm wrong, I think we should concentrate on version-1 first, instead of wanting the final solution immediately. I understand allot of things are wanted. But right now we have to start somewhere I suppose.

pprindeville · 2017-12-20T23:24:23Z

Can you git rebase -i HEAD~6 and then git push -f -u origin master to combine all your commits into one single one?

Signed-off-by: Jasper Scholte <[email protected]>

TimelessNL · 2017-12-21T09:06:14Z

Sorry I was unable to do a rebase, and did a soft reset instead. Hope this is sufficient.

karlp · 2017-12-21T10:14:40Z

net/ser2net/files/etc/init.d/ser2net

+
+	local enabled
+	config_get_bool enabled config 'enabled' '0'
+	[ "$enabled" -gt 0 ] || return 1


You want the enable to be on the use of the UCI conversion. There's still a gain to be had from an functional init script with the existing /etc/ser2net.conf file. particularly when you'll never be able to match the depth of availlable config in the UCI model. Like https://github.com/openwrt/packages/blob/master/net/mosquitto/files/etc/init.d/mosquitto#L186-L191

So what you meant is:
if enabled: the /etc/config/ser2net config file gets parsed to /tmp/ser2net.conf.
if disabled: the /etc/ser2net.conf file is used.

Is my assumption correct?

yes, that's what I was talking about at least.

Seems feasible to me. Do I have to provide a default /etc/ser2net.conf file or can I just check for its existence and fail if not found?

well, the package is already installing one, so I wouldn't bother doing any further checking.

Ok, agreed.

Unless the user explicitly removes it...

@pprindeville What if they remove the package itself?! where do you stop?

Let's not go over the top.

For someone concerned about security and not having a "fail open" policy, removing defaults to stop a (unconfigured) service from running is good security practice.

karlp · 2017-12-21T10:16:36Z

net/ser2net/files/etc/init.d/ser2net

+	local timeout
+	local options
+
+	config_get protocol $cfg protocol


this is called "state" in the ser2net configs, is there any real good reason to use a different word here?

This was requested by @mhei : #5302 (comment)

TimelessNL · 2017-12-23T01:11:57Z

Currently my time is limited but I'll resume this pull-request after the holidays. Happy holidays every one 👍

mhei · 2017-12-28T22:08:21Z

@TimelessNL: I'll pick your current version and extend it with my ideas. Please give me some time - I think I can present my version for discussion in one or two days.

TimelessNL · 2017-12-28T22:30:53Z

Thats great! You should have access to my branch if you need to.

pprindeville · 2017-12-29T01:47:52Z

@mhei Can you point me at it when you're ready to have people look at it? Thanks.

Signed-off-by: Michael Heimpold <[email protected]>

mhei · 2017-12-30T22:50:34Z

Please have a look at the branch I push a few seconds ago. Note, that it's not yet tested fully...

TimelessNL · 2018-01-03T22:36:28Z

@mhei Will do, should I close this pull request? Or should I merge your code?

mhei · 2018-01-04T09:34:48Z

@TimelessNL: no need to close/modify this PR, I will handle it during merge. Just review and/or test and add comments here.

Signed-off-by: Michael Heimpold <[email protected]>

TimelessNL · 2018-01-18T21:08:23Z

@mhei , see my comment on mhei@1dd54f5#commitcomment-26946566

Signed-off-by: Michael Heimpold <[email protected]>

TimelessNL mentioned this pull request Dec 19, 2017

luci-app-ser2net: created openwrt/luci#1480

Closed

pprindeville requested changes Dec 19, 2017

View reviewed changes

mhei self-assigned this Dec 20, 2017

karlp reviewed Dec 20, 2017

View reviewed changes

pprindeville requested changes Dec 20, 2017

View reviewed changes

mhei requested changes Dec 20, 2017

View reviewed changes

pprindeville requested changes Dec 20, 2017

View reviewed changes

TimelessNL changed the title ~~ser2net - Added support for config file.~~ ser2net: Added support for config file. Dec 20, 2017

ser2net: added support for config file.

e190a22

Signed-off-by: Jasper Scholte <[email protected]>

karlp reviewed Dec 21, 2017

View reviewed changes

mhei added a commit to mhei/packages that referenced this pull request Dec 30, 2017

ser2net: extend dynamic config generation (refs openwrt#5302)

1dd54f5

Signed-off-by: Michael Heimpold <[email protected]>

mhei merged commit e190a22 into openwrt:master Jan 7, 2018

mhei added a commit that referenced this pull request Jan 7, 2018

Merge branch 'ser2net' of https://github.com/mhei/packages (fixes #5302)

fb6fd48

Signed-off-by: Michael Heimpold <[email protected]>

salzmdan pushed a commit to salzmdan/packages that referenced this pull request Jan 8, 2018

ser2net: extend dynamic config generation (refs openwrt#5302)

bc226f7

Signed-off-by: Michael Heimpold <[email protected]>

jow- pushed a commit to jow-/packages that referenced this pull request Jan 15, 2018

ser2net: extend dynamic config generation (refs openwrt#5302)

c00d1e8

Signed-off-by: Michael Heimpold <[email protected]>

mhei added a commit that referenced this pull request Jan 18, 2018

ser2net: fix dynamic config generation (refs #5302)

e3e2c08

Signed-off-by: Michael Heimpold <[email protected]>

val-kulkov pushed a commit to val-kulkov/openwrt-packages that referenced this pull request Jan 26, 2018

ser2net: fix dynamic config generation (refs openwrt#5302)

4ad577a

Signed-off-by: Michael Heimpold <[email protected]>

lynxis pushed a commit to lynxis/packages that referenced this pull request Jan 3, 2019

ser2net: extend dynamic config generation (refs openwrt#5302)

e5d4bc7

Signed-off-by: Michael Heimpold <[email protected]>

lynxis pushed a commit to lynxis/packages that referenced this pull request Jan 3, 2019

ser2net: fix dynamic config generation (refs openwrt#5302)

3d3b3e0

Signed-off-by: Michael Heimpold <[email protected]>

ser2net: Added support for config file. #5302

ser2net: Added support for config file. #5302

Conversation

TimelessNL commented Dec 19, 2017

TimelessNL commented Dec 19, 2017

pprindeville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimelessNL Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pprindeville commented Dec 19, 2017

champtar commented Dec 19, 2017

pprindeville commented Dec 19, 2017

TimelessNL commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlp commented Dec 20, 2017

TimelessNL commented Dec 20, 2017 • edited Loading

TimelessNL commented Dec 20, 2017

pprindeville left a comment

Choose a reason for hiding this comment

pprindeville Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimelessNL Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhei commented Dec 20, 2017

TimelessNL commented Dec 20, 2017

pprindeville left a comment

Choose a reason for hiding this comment

pprindeville Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

pprindeville commented Dec 20, 2017

karlp commented Dec 20, 2017

pprindeville commented Dec 20, 2017

TimelessNL commented Dec 20, 2017

pprindeville commented Dec 20, 2017

pprindeville commented Dec 20, 2017

mhei commented Dec 20, 2017

TimelessNL commented Dec 20, 2017 • edited Loading

pprindeville commented Dec 20, 2017

TimelessNL commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimelessNL Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimelessNL Dec 20, 2017 •

edited

Loading

TimelessNL commented Dec 20, 2017 •

edited

Loading

pprindeville Dec 20, 2017 •

edited

Loading

TimelessNL Dec 21, 2017 •

edited

Loading

pprindeville Dec 20, 2017 •

edited

Loading

TimelessNL commented Dec 20, 2017 •

edited

Loading

TimelessNL Dec 21, 2017 •

edited

Loading

pprindeville Dec 21, 2017 •

edited

Loading