devlib: add monsoon instrument #100

joelagnel · 2017-03-17T19:59:10Z

Signed-off-by: Joel Fernandes joelaf@google.com

Signed-off-by: Joel Fernandes <joelaf@google.com>

setrofim · 2017-03-20T08:36:29Z

This does not implement the Instrument API as described here:

http://pythonhosted.org/devlib/instrumentation.html

The mode must be set appropriately
The measurements this instrument is capable of collecting must be advertised via channels
Depending on the mode, take_measurment and/or all of start/stop/get_data must be implemented as described in the docs.

Also please could you provide more information on what "monsoon" is. How does one get a hold of it, why would someone want to use it, etc?

derkling

@joelagnel the AEP instrument is quite similar to Monsoon, apart from having 3 channels instead of the single one available in the monsoon.

You should have a look at its code:
https://github.com/ARM-software/devlib/blob/master/devlib/instrument/energy_probe.py
and, for the monsoon device, just "hardcode" in its ctor the registration of the single channel available.

The sampling parameters also should probably be part of the InstrumentChannel registration. More details in the inline comments.

derkling · 2017-03-20T09:21:08Z

devlib/instrument/monsoon.py

+from devlib.utils.misc import which
+
+class MonsoonInstrument(Instrument):
+    MONSOON_BIN = "./tools/scripts/monsoon.par"


This tool/script is not part of the PR, can it be included?
If the license allows it, you should probably place it under:
devlib/bin/scripts

Otherwise, you can just use an additional mandatory monsoon_bin's __init__ parameter to specify its path.
Client code, e.g. LISA, can provide the path at creation time.

derkling · 2017-03-20T11:35:58Z

devlib/instrument/monsoon.py

+	super(MonsoonInstrument, self).__init__(target)
+
+	if not os.path.exists(MonsoonInstrument.MONSOON_BIN):
+		self.log_error("monsoon.par not found", exception=1)


Here, if possible, we should report some tip on how to get this binary.
This is what we did in LISA for the ACME cape:
https://github.com/ARM-software/lisa/blob/master/libs/utils/energy.py#L344
@setrofim: can this work for devlib as well?

derkling · 2017-03-20T11:36:49Z

devlib/instrument/monsoon.py

+	print "INFO: " + msg
+	self.logger.info(msg)
+
+    def __init__(self, target, device_entry=None):


What about s/device_entry/device/?
This _entry suffix seems not necessary...

derkling · 2017-03-20T11:37:48Z

devlib/instrument/monsoon.py

+	self.device_entry = device_entry
+	self.process = None
+
+	self.log_info('Found monsoon.par')


Do not report "it's working" type of comments, let's log only useful information and/or warnings/errors.

In devlib modules are "as much quite as possible". ;-)

derkling · 2017-03-20T11:38:47Z

devlib/instrument/monsoon.py

+	self.log_info('Found monsoon.par')
+
+    def reset(self):
+        self.log_info('Initiailzing MonsoonInstrument')


Some as above, not useful information to be reported here.

derkling · 2017-03-20T11:41:24Z

devlib/instrument/monsoon.py

+	self.run_monsoon(['--status'], timeout=3)
+	self.log_info("MonsoonInstrument ready for action")
+
+    def kill_monsoon(self, p):


For the Monsoon device we can probably support (at least initially) only the CONTINUOUS interface, thus you need to implement only 4 public methods: reset, start, stop and get_data.

Here is an example:
https://github.com/ARM-software/devlib/blob/master/devlib/instrument/energy_probe.py#L35

derkling · 2017-03-20T11:46:57Z

devlib/instrument/monsoon.py

+
+    # Returns pandas dataframe for monsoon output (time and current in mA)
+    def get_samples_sync(self, duration):
+	txt = self.run_monsoon(['--hz', '5', '--timestamp', '--samples', '2000000'], stream=1, timeout=duration)


These params should probably be exported somehow. Perhaps, since we have a single channel, we can specify them in the __init__?

In that case you can use them to configure the single InstrumentChannel which is going to be registered, as part of the instrument specific attrs:
https://github.com/ARM-software/devlib/blob/master/devlib/instrument/__init__.py#L142

derkling · 2017-03-20T11:52:44Z

@setrofim Monsoon is this single channel energy meter:
https://www.msoon.com/LabEquipment/PowerMonitor/
which is quite used within Google for all energy measurements related to the Android platform.

It is thus quite important to have support for this device within devlib and LISA.

bjackman · 2017-04-18T11:20:35Z

I have a simplified version of this functionality here: https://github.com/bjackman/devlib/blob/monsoon/devlib/instrument/monsoon.py

It seems to be working pretty well, not ready for merge though because:

I have only seen nonsense data so far (I don't think it's due to issues with my code or Monsoon, but let's be sure first!)
The Monsoon meter is only officially supported using Windows. Google have a Python tool ("monsoon.par") to communicate with it from Unix, and this code depends on it. Hopefully we can distribute it in Devlib but we need to figure out if there are issues with licensing, Google IP, or Monsoon IP.

setrofim · 2017-04-18T13:56:43Z

Hopefully we can distribute it in Devlib but we need to figure out if there are issues with licensing, Google IP, or Monsoon IP.

The path to the tool can be passed as an argument on __init__ , rather than be hard-coded in a class attribute in the mean time. This means the tool doesn't have to be included with devlib, and it would be up to the user to obtain it and point devlib to it.

This would make the instrument more awkward to use, so including the binary and setting the path in a class attribute would be preferable, but the above approach can be used to unblock the merging of the instrument in the mean time.

If/when the IP issues are resolved, and the tool can be included in devlib, the attribute can remain, and simply be made optional, defaulting to the internal version. That way backward compatibility with existing setups will be maintained.

bjackman · 2017-04-19T09:49:56Z

Some notes on this PR following from out-of-band chat with @joelagnel:

As @setrofim mentioned, need to get this compliant with the Instrument API, the branch I linked above aims to demo the bare minimum for that.
This returns a pandas DataFrame. However devlib doesn't currently depend on pandas. Instead, we should return CSV/MeasurementsCsv and then convert that to a DataFrame in a LISA wrapper. The current LISA energy meter classes don't do this but it's easy to add (I have a private branch that I should push at some point).
Will defer to @setrofim on this but I don't think set_voltage & set_current belongs in this class (which is for measuring), maybe it should be a Module, separate from the Instrument? Then I think the desired voltage and current could be parameters of the Platform? Not too sure but it's not a big deal.
I haven't yet been convinced of the need for loop in run_monsoon, when testing my version I never actually came across Monsoon flakiness when using it via devlib.
@joelagnel pointed out that Google's monsoon script is already public 😁 It has dependencies so I think the best cost/benefit is to just require users to install it themselves, like we do for caiman in energy_probe.py - @setrofim do you agree?

setrofim · 2017-04-19T12:23:50Z

Will defer to @setrofim on this but I don't think set_voltage & set_current belongs in this class (which is for measuring), maybe it should be a Module, separate from the Instrument? Then I think the desired voltage and current could be parameters of the Platform? Not too sure but it's not a big deal.

Form what I understood from the code, these set voltage/current in the monsoon device, not on the target. If that is the case, then the instrument is the right place for this, as they relate to monsoon device, rather than the target (even if they need to be adjusted to different values for different targets).

@joelagnel pointed out that Google's monsoon script is already public 😁 It has dependencies so I think the best cost/benefit is to just require users to install it themselves, like we do for caiman in energy_probe.py - @setrofim do you agree?

Agreed. It would be good to link to the the script in the documentation for this instrument.

bjackman · 2017-04-19T12:50:47Z

Form what I understood from the code, these set voltage/current in the monsoon device, not on the target. If that is the case, then the instrument is the right place for this, as they relate to monsoon device, rather than the target (even if they need to be adjusted to different values for different targets).

Actually Monsoon is a power supply as well as a measurement tool, so these really do set the voltage and (max?) current on the device. It's designed for testing mobile device prototypes by acting as a "fake battery".

setrofim · 2017-04-19T13:43:46Z

Actually Monsoon is a power supply as well as a measurement tool, so these really do set the voltage and (max?) current on the device. It's designed for testing mobile device prototypes by acting as a "fake battery".

Right, so these are the configurations of the Monsoon power supply, not of the target. So the set_voltage/set_current belong in the instrument. You could argue that the values for the voltage/current are properties of a particular device (what it expects to be supplied with), and so belong in the Platform, wherefrom they would be take by the instrument. However, given that this currently the only place where is this need, and that it's not something can actually be read from the target, I would suggest keeping them as the instrument parameters for now; If, in the future, we add support for some other power supply-type device to devlib that required the same configuration, then can move the setting into Platform.

bjackman · 2017-04-19T13:51:34Z

OK, sounds good.

setrofim · 2017-05-03T13:29:41Z

Superseded by #119

devlib: add monsoon instrument

2a80080

Signed-off-by: Joel Fernandes <joelaf@google.com>

joelagnel mentioned this pull request Mar 17, 2017

[RFC] Add a monsoon meter for devlib's monsoon instrument ARM-software/lisa#335

Closed

derkling suggested changes Mar 20, 2017

View reviewed changes

setrofim closed this May 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devlib: add monsoon instrument #100

devlib: add monsoon instrument #100

joelagnel commented Mar 17, 2017

setrofim commented Mar 20, 2017

derkling left a comment

derkling Mar 20, 2017

derkling Mar 20, 2017

derkling Mar 20, 2017

derkling Mar 20, 2017

derkling Mar 20, 2017

derkling Mar 20, 2017

derkling Mar 20, 2017

derkling commented Mar 20, 2017

bjackman commented Apr 18, 2017 •

edited

setrofim commented Apr 18, 2017

bjackman commented Apr 19, 2017 •

edited

setrofim commented Apr 19, 2017

bjackman commented Apr 19, 2017

setrofim commented Apr 19, 2017

bjackman commented Apr 19, 2017

setrofim commented May 3, 2017

devlib: add monsoon instrument #100

devlib: add monsoon instrument #100

Conversation

joelagnel commented Mar 17, 2017

setrofim commented Mar 20, 2017

derkling left a comment

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling Mar 20, 2017

Choose a reason for hiding this comment

derkling commented Mar 20, 2017

bjackman commented Apr 18, 2017 • edited

setrofim commented Apr 18, 2017

bjackman commented Apr 19, 2017 • edited

setrofim commented Apr 19, 2017

bjackman commented Apr 19, 2017

setrofim commented Apr 19, 2017

bjackman commented Apr 19, 2017

setrofim commented May 3, 2017

bjackman commented Apr 18, 2017 •

edited

bjackman commented Apr 19, 2017 •

edited