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

Separate remote control of simulated device #90

Merged
merged 45 commits into from
Sep 14, 2016
Merged

Conversation

MichaelWedel
Copy link
Contributor

@MichaelWedel MichaelWedel commented Sep 12, 2016

This fixes #39.

The simulated device can now be accessed through JSON-RPC 2.0 via ZMQ if enabled by the new -r in the startup script. Exposure happens on the object level, so classes don't have to be modified in any way to use this functionality.

On the client side, each exposed object is wrapped in a proxy that transforms access to properties, methods and plain attributes into JSON-RPC-requests. The proxy is more or less transparent and exceptions on the server side are re-raised on the client side if they are in exceptions (Python 2) or builtins (Python 3).

A new command line utility has been added, called control.py.

The new functionality is described in the readme.


def _get_zmq_req_socket(self):
context = zmq.Context()
return context.socket(zmq.REQ)
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this just be in __init__?

It seems to only be used here, and amounts to at most one more line in __init__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be in init, but testing got easier with this additional method, for tests where it's needed, this method is patched to return a Mock.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uff... that makes me a bit uneasy. Understandable, of course... just the thought of pulling apart code that belong together for the sake of tests... feels dirty.

Not a big deal here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the usual problem of testing things that involve networking, file access and so on...at least in my experience that is usually hard and/or inconvenient to do. In principle zmq.Context could be mocked here instead, I suppose the outcome would be similar.


parser = argparse.ArgumentParser(
description='A client to manipulate the simulated device remotely through a separate channel. The simulation must be started with the --rpc-host option.')
parser.add_argument('-i', '--ip', default='127.0.0.1', help='IP-adress of the host plankton is running on.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo: IP address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooops, thanks :) I'm blaming my native language processing firmware where one d is perfectly acceptable ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Always the firmware's fault... ;)

@@ -0,0 +1,224 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need this unless this file is meant to be executed directly from the command line.

rpc_server.py and both unit test files also shouldn't need this, I think.

Only control.py does need it, otherwise strange things happen when you try to run it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah this must be a leftover from the olden days where these files lived directly in the top level directory and were scripts to execute directly. Or it got in with the license headers.

Instead, it should be used as a base-class for dynamically created types
that mirror types on the server, like this:

proxy = type('SomeClassName', (ZMQJSONRPCObjectProxy, ), {})(socket, methods, prefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

The socket here is a bit confusing... shouldn't it be connection?

@MichaelWedel
Copy link
Contributor Author

Okay, this is what I propose for now:

from control_client import ControlClient, remote_objects

client = ControlClient(host='...', port='...')
objects = remote_objects(client)
chopper = objects['chopper']

For scripts that run once and do not wish to re-use the ControlClient-instance (like control.py) it can be shortened to:

objects = remote_objects(ControlClient(host='...', port='...'))
chopper = objects['chopper']

Objects can still be pulled out one by one from the ControlClient:

client = ControlClient(host='...', port='...')

# top level object, this should always work
top_level_object = client.get_object()

# object further down the hierarchy, works only if server side actually exposes that
chopper = client.get_object('chopper')

In principle the remote_objects is not needed, but it is convenient to retrieve all remote objects at once, so I'd like to keep it.

@MichaelWedel
Copy link
Contributor Author

I'm going to update the readme once we agree on whether the client side is okay.

An exception type for exceptions related to the transport protocol, i.e.
malformed requests etc.
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick / my fault: This pass could be removed (docstring already satisfies need for a statement)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these one-line changes which are obvious and don't need discussion, please feel free to just modify directly on the branch (I often do that via github directly).

@MichaelWedel
Copy link
Contributor Author

Let me know if you can live with this solution for the time being. I don't really see the dict-interface working very well at the moment, maybe that can be solved at some later point. Given that this solution does what we need at the moment I'd rather continue with the other aspects of the re-design and draw this to an end.

@MikeHart85
Copy link
Contributor

Looks perfectly fine to me. You're right, should move forward with some more bigger changes... we can work out details later if it becomes necessary.

@MikeHart85 MikeHart85 merged commit 13c257e into master Sep 14, 2016
@MikeHart85 MikeHart85 deleted the test_json_rpc branch September 14, 2016 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime control for simulation
2 participants