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

REPL script support #211

Merged
merged 3 commits into from Oct 10, 2020
Merged

REPL script support #211

merged 3 commits into from Oct 10, 2020

Conversation

attie
Copy link
Member

@attie attie commented Oct 8, 2020

This patchset will add support for run-repl --script ${FILENAME}, permitting a user to write a python script to directly interface with the iface.

I have also included an example script that will communicate with a BMP085 temperature and pressure sensor.

I wonder if it might also be beneficial for the script to have access to device, for functions like device.set_voltage(). This would permit power cycling the DUT from the script if / when necessary.

@attie attie changed the title REPL script REPL script support Oct 8, 2020
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Now that I see the code changes, I think it would be a lot nicer to have separate run-repl and run-script commands.

@attie
Copy link
Member Author

attie commented Oct 8, 2020

Sounds good to me - how do you feel about passing the device in too? Can I squash the commits into two?

@whitequark
Copy link
Member

how do you feel about passing the device in too?

Seems handy; if we do that in run-script, we should also do that in run-repl.

@attie
Copy link
Member Author

attie commented Oct 8, 2020

Would you like this warning to be emitted for run-script too?

@whitequark
Copy link
Member

whitequark commented Oct 8, 2020

Nope, that's not relevant for running scripts. The reason the warning exists is that some applets might want to add custom setup/teardown code specifically for REPLs (e.g. maybe the device will hang if you don't tear it down--this happens with EJTAG a lot). In a script you can just call the 1-2 functions required manually.

@attie
Copy link
Member Author

attie commented Oct 8, 2020

Understood - I suspected, but wanted to check.

@mwick83
Copy link

mwick83 commented Oct 8, 2020

Just my thoughts (as I seem to have triggered this feature being worked on): I think having "device" available as well would be great. Yes, power cycling would be a good use-case. Also, bringing the device + DUT back into safe state seems to be nice to have feature from within a script.

@attie
Copy link
Member Author

attie commented Oct 8, 2020

@mwick83 - there is an example script here: https://github.com/attie/glasgow/blob/repl-script/software/scripts/i2c-bmp085.py, it also demonstrates powering down the DUT at the end.

@mwick83
Copy link

mwick83 commented Oct 8, 2020

@mwick83 - there is an example script here: https://github.com/attie/glasgow/blob/repl-script/software/scripts/i2c-bmp085.py, it also demonstrates powering down the DUT at the end.

Neat! That's actually what I had in mind, when I asked about such a feature :-)

software/glasgow/cli.py Outdated Show resolved Hide resolved
@whitequark
Copy link
Member

I don't like having an example script for a device we do not otherwise support. It's enough code that it needs to be maintained, and it's not maintainable.

@electroniceel
Copy link
Member

I don't like having an example script for a device we do not otherwise support.

yeah. That part is a bit strange.

OTOH, I like having an example for a I2C target, as I guess that this will be a very common usecase for the script feature.

So maybe strip down the example so that it doesn't do anything other than reading out and printing the contents of one of the registers? That should reduce the code a lot and make it maintainable even without a sample device available. The important bits, like what to import and how to access the iface and so on, are still there.

Another idea would be to make the example fully self contained, usable with just a Glasgow and nothing else. Like toggling one of the pins.

@whitequark
Copy link
Member

If the example was working with something like PCF8574 then there would be no such problem.

@attie
Copy link
Member Author

attie commented Oct 9, 2020

it's not maintainable

... yeah, the complexity ran away from me there, sorry. (I wanted to give an example for something I had to hand and could prove worked).

I've replaced it with an example for a PCF8574 - though I don't have one, I think what I've written should work.
If it's still too much complexity for a "simple example", then I can strip it down further.

@whitequark
Copy link
Member

whitequark commented Oct 9, 2020

If it's still too much complexity for a "simple example", then I can strip it down further.

Yep, please do. I would just show a single read and a single write. No classes, no LED tricks, nothing.

@whitequark
Copy link
Member

I've cherry-picked your other commits. I'm still not happy with the example so I haven't merged it. I don't know when I'll find time to discuss or rewrite it, so feel free to keep the PR open or close it.

@attie
Copy link
Member Author

attie commented Oct 9, 2020

Fair enough / sorry. I think an example is always handy - it gives a starting point.
Happy to discuss when you have the spoons / time... no pressure.

I could trim it back even further, but I think it starts to lose function at that point... and a non-functional (or an "it works if you're careful") example is potentially a bad thing...

Is the issue with the fact that it's an example (which adds maintenance), or the contents of the example?

Is this the sort of thing you have in mind?

# set all pins high, so that their inputs may be read
print('p: 0x{:02X}'.format(await iface.read(0x20, 1)))

# set a pattern on the pins
await iface.write(0x20, [ 0x55 ])

# power down the device after use
await device.set_voltage("AB", 0)

@whitequark
Copy link
Member

Is this the sort of thing you have in mind?

Yep, pretty much. I also wanted to reformat the header to follow the writing conventions elsewhere, and make the messages that are printed a bit more informative.

@attie
Copy link
Member Author

attie commented Oct 9, 2020

Last try, and then I'll leave it to you if that's okay?

@whitequark
Copy link
Member

Thanks! This is perfect.

@whitequark whitequark merged commit 5307005 into GlasgowEmbedded:master Oct 10, 2020
@attie attie deleted the repl-script branch October 10, 2020 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants