ntoll / uflash

A module and command to easily flash Python onto the BBC's micro:bit device.
http://micropython.org/
MIT License
101 stars 27 forks source link

Implement find_microbit for posix & nt #1

Closed funkyHat closed 8 years ago

funkyHat commented 8 years ago

So I've completely inlined the posix case because it's really simple.

The nt case is awkward and I've made a function within the if statement because it seemed silly to do stuff like allocating ctypes memory that's only for windows at the top level.

I'm not sure I prefer this or not to putting at least the windows code in a separate module. What do you think?

My 3 separate modules are still on my master branch under spikes

ntoll commented 8 years ago

This is great..! Thank you.

A couple of things:

I've asked Tim Golden to take a look at the Windows stuff since I don't have the domain knowledge to appropriately review that code. ;-)

tjguk commented 8 years ago

There's a bit of a weigh-up here: this approach should certainly work (although for 2.x/3.x compat I'd hardcode the list of drive letters). The alternative is to use command-line tools such as "vol" or "wmic" and parse the output. (eg "wmic logicaldisk get caption,volumename" or "vol j:"). But then you're at the mercy of setups which lack those tools and/or possible i18n issues when parsing.

I think we should go with the ctypes solution used here and possibly use wmic-parsing for tests. (or vol-parsing)

tjguk commented 8 years ago

@funkyHat I'm not sure whether you understand the Windows code (and can therefore annotate it yourself) or whether you've cargo-culted it, in which case I'm happy to write some comment blocks.

ntoll commented 8 years ago

@tjguk I agree with hard coding 'A->Z' letters. I didn't realise string.uppercase was missing from Python3.

ntoll commented 8 years ago

@funkyHat apologies for the splurge of comments. Happy to do this work over the weekend if you don't have bandwidth. :-D

tjguk commented 8 years ago

It's actually called "string.ascii_uppercase" but easy enough to type 26 letters!

ntoll commented 8 years ago

So it is... ;-) I suggest we use string.ascii_uppercase because it's only 22 letters and it's good to flag this sort of thing exists in the standard lib. :-P

tjguk commented 8 years ago

But that's 3.x only. What are we targetting?

funkyHat commented 8 years ago

I'd rather check for string.ascii_uppercase and use string.uppercase if it's not there than type out A-Z :)

ntoll commented 8 years ago

@tjguk Python 3 (of course). ;-)

ntoll commented 8 years ago

Please see: https://github.com/funkyHat/uflash/pull/1 for my refactoring of this. Comments welcome (as are tests). ;-)

funkyHat commented 8 years ago

So I've managed to do most of this locally without noticing that you've done it too... annoying. Looking at your PR now.

ntoll commented 8 years ago

I've finished the work on this branch (including some "interesting" tests...). I can confirm it works on both Linux and Windows. Pending OSX check can be fixed (if required) in a later bug fix.

THANK YOU MATT!!!! :-)

ntoll commented 8 years ago

@funkyHat so sorry..! I interpreted your silence as "keep going" rather than "I don't check my email". ;-)

All feedback most welcome. Put simply, this work needs doing ASAP and I'm about 45 mins away from a 0.9 release that does exactly what it says on the tin.

funkyHat commented 8 years ago

@ntoll No worries, I should have at least had a quick glance here before getting going. All 8 written tests pass on OS X on Python 3 :). Python 2 is missing mock, but.... I've just seen that you're requiring at least 3.3 so I guess that's fine :D