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

luci-app-ser2net: created #1480

Closed
wants to merge 1 commit into from
Closed

Conversation

TimelessNL
Copy link

This changeset will add support for ser2net in luci-app-ser2net.

Note: This depends on openwrt/packages#5302 to be accepted first.

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

Signed-off-by: Jasper Scholte <NightNL@outlook.com>
@TimelessNL TimelessNL changed the title Added luci-app-ser2net which add support for ser2net. luci-app-ser2net: Added Dec 20, 2017
@TimelessNL TimelessNL changed the title luci-app-ser2net: Added luci-app-ser2net: created Dec 20, 2017
Copy link
Member

@feckert feckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The translations are very long you should wrap them on the 80 column boundary
Nice work :-)

@@ -0,0 +1,14 @@
#
# Copyright (C) 2008-2017 The LuCI Team <luci@lists.subsignal.org>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I know please change the Copyright notice to

Copyright (C) 2018 Jasper Scholte NightNL@outlook.com

@@ -0,0 +1,7 @@
module("luci.controller.ser2net", package.seeall)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add the following header on top off the file

-- Copyright (C) 2018 Jasper Scholte NightNL@outlook.com
-- Licensed to the public under the Apache License 2.0.

@@ -0,0 +1,58 @@
m = Map("ser2net", translate("ser2net"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add the following header on top off the file

-- Copyright (C) 2018 Jasper Scholte NightNL@outlook.com
-- Licensed to the public under the Apache License 2.0.

translate("The ser2net allows telnet and tcp sessions to be established with a unit's serial ports.<br/>"))

function m.on_after_commit(self)
luci.sys.call("/etc/init.d/ser2net enable 1\>/dev/null 2\>/dev/null")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use

luci.sys.init.enable("ser2net")


function m.on_after_commit(self)
luci.sys.call("/etc/init.d/ser2net enable 1\>/dev/null 2\>/dev/null")
luci.sys.call("/etc/init.d/ser2net restart 1\>/dev/null 2\>/dev/null")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You sould use

luci.sys.call("/etc/init.d/ser2net restart >/dev/null 2>&1")

@arturioxas
Copy link

Changes you submitted seem to write to configuration that only luci understands. In my opinion it seems unreliable, because:

  1. ser2net by default uses /etc/ser2net.conf. Therefore initializing won't do anything by default because setting are setting in uci format in uci compatible file
  2. changing /etc/ser2net.conf or using example setting won't be reflected in frontend application

@TimelessNL
Copy link
Author

True. I forgot about this request. The implementation has changed in OpenWrt. So I have to refactor this pullrequest. I'll close it for now.

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.

3 participants