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

[IMP] IoT : IoT refactoring - With longpooling #30440

Open
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
6 participants
@qle-odoo
Copy link
Contributor

qle-odoo commented Jan 22, 2019

Improve the implementation of IoT.
Allow both-direction communication between iot box and devices.

task : 1906910

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the seen 🙂 label Jan 22, 2019

@qle-odoo qle-odoo requested review from sle-odoo , switch87 and jco-odoo Jan 22, 2019

@qle-odoo qle-odoo added 12.0 IoT labels Jan 22, 2019

@C3POdoo C3POdoo added the RD label Jan 22, 2019

@robodoo robodoo added the CI 🤖 label Jan 22, 2019

@switch87
Copy link
Contributor

switch87 left a comment

did not review everything, here is already some feedback

Show resolved Hide resolved addons/hw_drivers/controllers/driver.py Outdated
Show resolved Hide resolved addons/hw_drivers/controllers/driver.py Outdated
Show resolved Hide resolved addons/hw_drivers/controllers/driver.py
Show resolved Hide resolved addons/hw_drivers/controllers/driver.py Outdated
Show resolved Hide resolved addons/hw_drivers/controllers/driver.py
Show resolved Hide resolved addons/hw_posbox_homepage/controllers/main.py Outdated
Show resolved Hide resolved addons/hw_drivers/controllers/driver.py Outdated
Show resolved Hide resolved addons/hw_drivers/controllers/driver.py Outdated
addons/hw_drivers/controllers/driver.py Outdated
if device_id in all_devices:
return all_devices[device_id].action(data)
else:
return {'message': _('Device %s not found' % device_id)}

This comment has been minimized.

@switch87

switch87 Jan 23, 2019

Contributor

return False or with a response object with code 204

odoo/odoo/http.py

Lines 741 to 758 in 8f2267b

""" Handler for the ``http`` request type.
matched routing parameters, query string parameters, form_ parameters
and files are passed to the handler method as keyword arguments.
In case of name conflict, routing parameters have priority.
The handler method's result can be:
* a falsy value, in which case the HTTP response will be an
`HTTP 204`_ (No Content)
* a werkzeug Response object, which is returned as-is
* a ``str`` or ``unicode``, will be wrapped in a Response object and
interpreted as HTML
.. _form: http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2
.. _HTTP 204: http://tools.ietf.org/html/rfc7231#section-6.3.5
"""

Show resolved Hide resolved addons/hw_posbox_homepage/controllers/main.py Outdated
Show resolved Hide resolved addons/hw_posbox_homepage/controllers/main.py Outdated
Show resolved Hide resolved ...osbox/overwrite_after_init/usr/local/lib/python3.5/dist-packages/v4l2.py Outdated
Show resolved Hide resolved ...oint_of_sale/tools/posbox/overwrite_before_init/etc/init_posbox_image.sh Outdated
@switch87
Copy link
Contributor

switch87 left a comment

to make this refactor a little more complete I personally would like to see some extra changes

fstab

  • write all mounts to fstab, no remounts at boot.
  • mount / with rw and noatime options and remove the root_bypass_ramdisks.
  • no ramdisk for /etc, there is no reason to do so.
  • ramdisk for /var/tmp and /var/log, not for /var.
  • do not be to shy on assigning size to ramdisks. size is the maximum allocated ram for the mount point, it will probably never really use this.

There are many articles about optimizations of ssd/sd on the internet, so probably there is more you can do then the above, but these changes will get rid of the countless sudo mount and bypass commands all over the code.

updating

at update it would be nice if after_init scripts will be ran again so new dependencies will be automatically installed et cetera. A good example for this is the Wordline branch that needs some extra initialisation, If this kind of drivers make it to the official version, as it is now, it would require a new iso to be flashed. This adds a complexity for the end-user we can easily prevent.

@jco-odoo

This comment has been minimized.

Copy link
Contributor

jco-odoo commented Jan 23, 2019

@switch87

This comment has been minimized.

Copy link
Contributor

switch87 commented Jan 23, 2019

Do not think noatime is the only thing we need to do to prevent the sd card from breaking, but we could certainly check further.

If you put all log and tmp dirs in ramdisk it should be. remember Linux and Windows are very different from each other on writing policies. In Linux most directories are static. You could maybe make the /home/pi folder read-only but I cannot imagine what process would write that lot of data to it to be bad for the sd card.

One not to long article about it: https://haydenjames.io/increase-performance-lifespan-ssds-sd-cards/
For more depth I would recommend sites like the Gentoo or Arch wiki, If you are really interested.

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch Jan 29, 2019

@robodoo robodoo removed the CI 🤖 label Jan 29, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch to cb7bbe0 Jan 29, 2019

@robodoo robodoo added the CI 🤖 label Jan 29, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from cb7bbe0 Jan 30, 2019

@robodoo robodoo removed the CI 🤖 label Jan 30, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch Jan 30, 2019

@robodoo robodoo added the CI 🤖 label Jan 30, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch to d4f2a96 Jan 30, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 30, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from d4f2a96 to 35ba36b Feb 4, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 4, 2019

token = token.split('|')[1]
reboot = 'noreboot'
path = os.getenv("HOME") + '/odoo/addons/point_of_sale/tools/posbox/configuration/connect_to_server.sh'
subprocess_call([path, url, iotname, token, reboot])

This comment has been minimized.

@sle-odoo

sle-odoo Feb 13, 2019

Contributor

check for the returncode?

'value': device.value,
'data': device.data,
})
session['event'].set()

This comment has been minimized.

@sle-odoo

sle-odoo Feb 13, 2019

Contributor

do you expect setting multiple events at a time?
if not, maybe returning a value here would help the caller to know if deviceChanged did or didn't do something

@switch87

This comment has been minimized.

Copy link
Contributor

switch87 commented Feb 13, 2019

@sle-odoo please use the start review button and send everything together. Everyone following this pr gets mails for the comments you post.

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from aac2fad to 84843e3 Feb 14, 2019

@robodoo robodoo removed the CI 🤖 label Feb 14, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from 84843e3 to 9897f06 Feb 14, 2019

class StatusController(http.Controller):
@http.route('/hw_drivers/action', type='json', auth='none', cors='*', csrf=False)
def action(self, device_id, data):
if device_id in iot_devices:

This comment has been minimized.

@sle-odoo

sle-odoo Feb 14, 2019

Contributor
iot_device = iot_devices.get('device_id')
if iot_device
    return iot_device.action(data)
return False

does the webclient really wants to know that the device id was not found? i'm not sure

This comment has been minimized.

@sle-odoo

sle-odoo Feb 14, 2019

Contributor

i would make the request fail + add a readable message

if req['event'].wait(58):
req['event'].clear()
return_value = req['queue'].copy()
return return_value

This comment has been minimized.

@sle-odoo

sle-odoo Feb 14, 2019

Contributor

i don't remember what al said, we talked about keeping only /event and removing /action

@robodoo robodoo added the CI 🤖 label Feb 14, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from 9897f06 to d3fa0e5 Feb 15, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 15, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from d3fa0e5 to 8985a65 Feb 15, 2019

@robodoo robodoo removed the CI 🤖 label Feb 15, 2019

@qle-odoo qle-odoo force-pushed the odoo-dev:12.0-iot-V2 branch from 8985a65 to 0dd507f Feb 15, 2019

@robodoo robodoo added the CI 🤖 label Feb 15, 2019

return {'message': _('Device %s not found') % device_id}

@http.route('/hw_drivers/event', type='json', auth='none', cors='*', csrf=False)
def event(self, requests):

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

why requestS
explain in the docstring this argument (dict with request_id and device_id key)

iotname = ''
token = b64decode(token).decode('utf-8')
url = token.split('|')[0]
token = token.split('|')[1]

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

url, token = token.split('|')

token = token.split('|')[1]
reboot = 'noreboot'
path = os.getenv("HOME") + '/odoo/addons/point_of_sale/tools/posbox/configuration/connect_to_server.sh'
subprocess.call([path, url, iotname, token, reboot])

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

use directly '' as iotname and 'noreboot'
these variables are unused

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

check subprocess return code?

reboot = 'noreboot'
path = os.getenv("HOME") + '/odoo/addons/point_of_sale/tools/posbox/configuration/connect_to_server.sh'
subprocess.call([path, url, iotname, token, reboot])
m.send_alldevices()

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor
m.send_alldevices()  # Make the IotBox send its information to the Odoo database

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

and remove the else + unindent


class BtMetaClass(type):
class MetaClass(type):

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

rename into DriverMetaClass

This comment has been minimized.

@sle-odoo

sle-odoo Feb 15, 2019

Contributor

add a docstring to this class ; explain that it's just a hook to register the driver into the drivers list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment