Re: [Qemu-devel] [RFC] qapi: command category to stimulate high-level machine devices

[Eric Blake]

On 06/01/2018 10:16 PM, Steffen Görtz wrote:
> From: Steffen Goertz <address@hidden>
>
> Add a new category "stimulate" to host commands that
> act upon high-level devices associated with machines/boards.
>
> This is patch is part of an effort to add support for BBC:microbit
> educational computer to QEMU.
>
> The microbit board, as many other microcontroller boards,
> contains typical trivial (digital) input and output options such as buttons and 
> LEDs,
> but also more sophiscated possibilities such as analog inputs and

s/sophiscated/sophisticated/

> complex dedicated sensor ICs that are connected to the microbit machine
> by means of inter-ic bus (i2c).
> These devices interact with peripheral devices of the microcontroller
> in use, for example with the GPIO peripheral of the used NRF51.
>
> One aim of our efforts is to create a user interface that models the
> look and feel of the physical microbit board and lets the user interact
> with its inputs and provide feedback on its outsputs.

s/outsputs/outputs/

> For this it is necessary to transmit user inputs such as the pressing of a
> button to the machine.
> In return, when the state of an output is changed on the machine, this
> change needs to be reflected in the user interface.
>
> At the moment, there are only a few QMP commands that provide user input to the
> machine, eg. send-keys, input-button. These commands require very
> high-level concepts, which are not really suitable for applications that
> microcontrollers are typically used in in.
>
> This RFC is meant to start a discussion on how to provide stimuli to
> low-complex inputs and outputs typically found in embedded
> microcontroller applications. To the best of my knowledge, there are
> currently no examples of how to provide such stimuli to either SoC
> device peripheral or machine level concepts like pushbuttons and LEDs.
>
> To my understanding, the following concepts/apis are needed:
> - QMP commands to send stimuli to machine level concepts like
> pushbuttons
> - Machine level devices that receive these stimuli and act as an adapter
> to manipulate the associated SoC peripheral devices
> - For outputs, machine level devices are needed that observe changes in
> SoC peripherals and emit QMP events that clients can receive
>
> This patch adds a new qmp command "buttons-set-state" to update the
> push-down state of arbitrarily identified buttons (string identifier).

An arbitrary string has the most flexibility, but also the least 
discoverability.  Is there any way to enumerate which strings are valid, 
and/or make the button names an enum instead of an open-coded string?

> The handler of this command is mocked to set the machines SoC gpio/irq
> lines associated with these buttons on the machine by means of object
> property aliases. This is just a mock until a proper concept/api is
> agreed upon.
>
> As i recently joined the QEMU community as part of GSoC, i am still a
> greenhorn to the codebase and i am really excited to discuss potential
> concepts and APIs to realize the described features.

This last paragraph...

> Based-on: <address@hidden>
> Signed-off-by: Steffen Goertz <address@hidden>
> ---

...would fit better here, after the --- separator.  It is useful to 
reviewers now, but a year from now when reading the git history (and 
hopefully after you are a regular contributor), it won't be as relevant 
to understanding this particular patch or your contributions in general.

At this point, I haven't yet decided if this interface addition is the 
best way to approach things, but if we take it without a redesign, it 
needs at least the following tweaks:

> +++ b/qapi/stimulate.json
> @@ -0,0 +1,24 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# @ButtonPress:
> +#
> +# @identifier:  Name of the button.
> +# @pushed_down: State of the button.

In QMP, we prefer naming things with dash, as in 'pushed-down'.  Or, if 
it is easier, you could create an enum with values 'down' and 'up' and 
use that, instead of 'bool', as the type, at which point this variable 
might be better named as 'position' instead of 'pushed-down'.

> +#
> +##

Missing a tag of:

Since: 3.0

> +{ 'struct': 'ButtonPress',
> +  'data': {
> +    'identifier': 'str',
> +    'pushed_down': 'bool'
> +     } }
> +
> +##
> +# @buttons-set-state:
> +#
> +# @buttons: List of updated button states.
> +#
> +# Returns: nothing in case of success
> +##

Missing a Since: tag.

> +{ 'command': 'buttons-set-state',
> +  'data': { 'buttons': ['ButtonPress'] } }

Doesn't let a user set button state (presumably that's for a later 
patch), but at least answers the question on introspecting the names of 
which buttons have state.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org