Use target connections as contexts. #465

setrofim · 2020-01-30T07:57:27Z

Make it easier to use multiple connections to the target from the same thread.

setrofim · 2020-01-30T07:59:23Z

Cc @douglas-raillard-arm: this should be largely orthogonal to #450, I believe, but FYI, just in case.

marcbonnici · 2020-01-30T09:25:03Z

doc/target.rst

+   Set the Target's connection for the current thread to the one specified
+   (typically, one that has previously been returned by the call to
+   ``get_connection``). Returns the old connection to the current thread -- it
+   is up to the coller to keep track of it and restore it if they wish.


/s/coller/caller.

marcbonnici · 2020-01-30T09:29:03Z

doc/connection.rst

+    target.execute('ls')  # uses conn rather than the default connection.
+```
+
+If the connection object is being uses withn another function, you do not need


marcbonnici · 2020-01-30T09:29:29Z

doc/connection.rst

+    target.execute('ls')  # uses conn rather than the default connection.
+```
+
+If the connection object is being uses withn another function, you do not need


/s/withn/within

marcbonnici · 2020-01-30T09:46:18Z

doc/connection.rst

+
+.. _multiple-connections:
+
+Multipe Connections With One Target


Is it worth adding a note somewhere that Gem5Connections and TelnetConnections do not support this?

Good point. I'll add a note for Gem5Connection. TelnetConnection actually derives from SshConnection so it would be supported.

In which case for TelnetConnection, a call to the BaseConnection initi method will also need to be added.

Ah, yes. Good catch. Fixed.

marcbonnici · 2020-01-30T09:51:15Z

doc/connection.rst

@@ -249,3 +249,32 @@ The only methods discussed below are those that will be overwritten by the
 .. method:: _wait_for_boot(self)

    Wait for the gem5 simulated system to have booted and finished the booting animation.
+


Update the info on line 19 about a common base?

Add a method to set the connection for the current thread within the target. This method returns the connection that was previously set.

douglas-raillard-arm · 2020-01-30T11:08:32Z

You might want to take the patches that fixes thread local variables to store the connection first, they somehow conflicts, especially on the set_connection() side that is already handled by tls_property out of the box:
d8f9fac
aa3b0e3

I can submit them in a separate PR if needed.

There is also a ConnectionBase class introduced:
d7908d1
But it should be straightforward to merge the features of both as they don't seem to overlap.

setrofim · 2020-01-30T12:17:01Z

There is also a ConnectionBase class introduced:
d7908d1
But it should be straightforward to merge the features of both as they don't seem to overlap.

Ah cool, I'll hold off on this then, until that is merged, and will then add the changes to that. One nit -- could you rename the file to connection.py (singular) to be consistent with the other files.

Add a base class for connection objects that implements the context manager interface and keeps track of the connection's target (which gets set by Target.get_connection). When used as a context manager, the connection substitutes itself for the target's default connection for the thread within the context using Target.set_connect(). The old connection is restored upon exiting the context.

douglas-raillard-arm · 2024-05-08T11:25:17Z

I think this PR is stale as quite a lot of things has changed since then in that area, but it would be pretty easy to implement these days with something like that:

class Target:
     ...
     @contextlib.contextmanager
     def with_connection(self, conn):
        old = self.conn
        try:
            self.conn = conn
            yield
        finally:
            self.conn = old

self.conn is defined using devlib.utils.misc.tls_property that provides a thread-local value (and creates a value when used for the first time in a given thread). It supports assignment (__set__) and that assignment will be thread-local as well. The main issue with that implementation is that it will create the old connection if it does not exist yet, but that would be easy to fix by extending the tls_property API to optionally let it raise instead of creating a new instance.

marcbonnici reviewed Jan 30, 2020

View reviewed changes

target: add set_connection()

b697f99

Add a method to set the connection for the current thread within the target. This method returns the connection that was previously set.

setrofim force-pushed the conn-context branch from 252a4ff to 77cada6 Compare January 30, 2020 10:57

setrofim force-pushed the conn-context branch from 77cada6 to 3327ea1 Compare January 30, 2020 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use target connections as contexts. #465

Use target connections as contexts. #465

setrofim commented Jan 30, 2020

setrofim commented Jan 30, 2020

marcbonnici Jan 30, 2020

marcbonnici Jan 30, 2020

marcbonnici Jan 30, 2020

marcbonnici Jan 30, 2020

setrofim Jan 30, 2020

marcbonnici Jan 30, 2020

setrofim Jan 30, 2020

marcbonnici Jan 30, 2020

douglas-raillard-arm commented Jan 30, 2020 •

edited

Loading

setrofim commented Jan 30, 2020

douglas-raillard-arm commented May 8, 2024


		.. _multiple-connections:

		Multipe Connections With One Target

		@@ -249,3 +249,32 @@ The only methods discussed below are those that will be overwritten by the
		.. method:: _wait_for_boot(self)

		Wait for the gem5 simulated system to have booted and finished the booting animation.

Use target connections as contexts. #465

Are you sure you want to change the base?

Use target connections as contexts. #465

Conversation

setrofim commented Jan 30, 2020

setrofim commented Jan 30, 2020

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

douglas-raillard-arm commented Jan 30, 2020 • edited Loading

setrofim commented Jan 30, 2020

douglas-raillard-arm commented May 8, 2024

douglas-raillard-arm commented Jan 30, 2020 •

edited

Loading