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

Extract scripts from non-uflash'd hex files #16

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

L0C4RD
Copy link

@L0C4RD L0C4RD commented Jun 7, 2016

extract_script() is extended to be able to handle hex files that haven't necessarily been generated directly by uflash, and have perhaps been produced by other means (e.g. a hex dump via SWD.) It still works identically for uflash-generated hex files.

@@ -1,3 +1,3 @@
Nicholas H.Tollervey ([email protected])
Matt Wheeler ([email protected])

L0C4RD ([email protected])
Copy link
Owner

Choose a reason for hiding this comment

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

Given this is software written for a device aimed at kids, we need your real name. No need to be shy about your contribution either. ;-)

Copy link
Author

Choose a reason for hiding this comment

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

@ntoll : Real name now added!

@L0C4RD
Copy link
Author

L0C4RD commented Jul 12, 2016

I've slightly modified extract_script() to allow it to work across different versions of the microbit runtime. In all of its previous incarnations, it'd only work when the runtime in the hex file being interrogated was identical to that in the heredoc in uflash.py - now it ought to keep working for all past and future versions of micropython.

In other words: updating the microbit micropython runtime will no longer cause extract_script() to fail.

@@ -1,3 +1,3 @@
Nicholas H.Tollervey ([email protected])
Matt Wheeler ([email protected])

Edmond Locard ([email protected])
Copy link
Owner

Choose a reason for hiding this comment

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

Very cute. If, by chance, you really are named after the famous French forensics expert please link me to some sort of online presence I can use to verify who you are. Otherwise, pseudonyms are no more helpful than an anonymous gmail username, even for people who probably work in some sort of infosec capacity. :-)

Copy link
Author

@L0C4RD L0C4RD Aug 16, 2016

Choose a reason for hiding this comment

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

LinkedIn: linkedin.com
Twitter: twitter.com
Talk I did recently (think my name is on one of the slides): youtu.be

I hope it's understandable that I try to minimise my online presence - the work that I do is quite often misrepresented in the media, and on more than one occasion I've had people make threats to me, both across the internet and in person. Unfortunately, having a weird name makes it that much easier to find me.

Because of all this, I don't get as many opportunities to work on amazing community projects as I'd like. However after reading your contributions guidelines here, and being really impressed by what seemed to be a radically inclusive outlook on contributions, I thought you guys might be willing to accept some of my work, even lacking, as I am, a huge public presence - please prove me right!

pokes (as it's been a little while) : @ntoll

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @L0C4RD and many thanks for your patience.

I completely understand and totally sympathise with your reasons for a low profile. I hope you, in turn, can see why I may also have been a tad paranoid about accepting contributions for code relating to a project aimed at kids from an anonymous third party.

Given the links you provided above I'm completely happy to accept your patch and any further contributions you may make!

I really enjoyed your presentation. It had me grinning like a Cheshire Cat. I have a few questions for you about the talk and I wonder if you might contact me via ntoll ntoll org.

Also, you're right about our intentions regarding contributions: radical inclusion is one way to say we want to work in a community that reflects all of us. Diversity is a strength!

Finally, there's a few things I want to fix in the code you've submitted. Did you make check..? ;-) There's a bunch of code style warnings that I've fixed locally on my machine. Also, there are some Python 2/3 compatibility issues (StringIO etc...).

Also, you've introduced a dependency on the third party IntelHex module. Does this need to be used? One of the "features" of uflash is that it's stand-alone so you could just drop the file into any other project (like Mu) and "it should just work" (tm).

Thoughts..?

Copy link
Owner

Choose a reason for hiding this comment

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

Just noticed your email address in the AUTHORS file. I'll drop you a line.

Copy link
Author

Choose a reason for hiding this comment

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

Aha - I did not run make check! Apologies, I'll do that on my end and try to get those warnings cleaned up for you, along with those compatibility problems.

The IntelHex module was just a shortcut for quickly interpreting Intel Hex files, but everything it accomplishes can be achieved with a dict and some parsing code. I'll implement this and pop it into the next commit, so it shouldn't jeopardise uflash's standalone-ness. 😄

Copy link
Owner

Choose a reason for hiding this comment

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

Great stuff and many thanks. Looking forward to the revised PR. :-)

@L0C4RD
Copy link
Author

L0C4RD commented Oct 3, 2016

Removed both the import IntelHex and the import StringIO. Functionality required from each of them is now encapsulated in map_memory(), which builds a representation of the device's flash memory as a Python dictionary.

I'm still getting three errors from make test, however these seem to only occur during testing - when I run the actions that make test is attempting to simulate, I get the output that the test script is expecting.

@ntoll
Copy link
Owner

ntoll commented Oct 5, 2016

I'll take a look and see what fresh eyes bring to the testing problem. Thanks for this work! :-)

@ntoll
Copy link
Owner

ntoll commented Oct 16, 2016

Hi @L0C4RD,

So I've finally managed to find some time to look into this. There are conflicts with the current master - could you perhaps rebase and resolve the conflicts and put the work into a single commit? (git pull master then git rebase -i master).

Also, if you make check you'll see that there are quite a number of code style issues (that should be relatively easy to fix). Most importantly for me is that there is NOT any doc string on the map_memory function. Essentially, could you just copy how all the other functions are documented please?

Assuming the above and the tests can be fixed (something I'll look into) then I believe we're good to merge!

Thanks for your patience.

N.

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.

2 participants