CircuitPython now has struct instead of ustruct by mrmcwethy · Pull Request #302 · adafruit/circuitpython

mrmcwethy · 2017-10-02T19:13:19Z

This is my attempt at moving micropython ustruct into shared_bindings and shared_module for both atmel and esp8266

tannewt

Overall, this is looking really good! Just a couple style things along with questions about CPython interop.

tannewt · 2017-10-02T22:14:31Z

Add yourself here @mrmcwethy

tannewt · 2017-10-02T22:15:30Z

Are these still supported? This is the kinda of thing I don't want to support because it won't port to CPython.

I don't know of any extensions other than some slight behavior differences that i already have pointed out. I took the text from the original code, so i will remove the words ,'with some extensions'

I looked at the micro python documentation. They do not document these extensions. We won't either. There is some code that is looking for 'S'. I don't yet see any for 'O'. I can delete the 'S' code.

tannewt · 2017-10-02T22:18:07Z

No need for the _internal suffix. Its implicit with shared_modules.

tannewt · 2017-10-02T22:21:10Z

We should probably not pass this test because its checking that MicroPython doesn't match CPython.

ok, another difference

tannewt · 2017-10-02T22:22:54Z

I know its a weird but I think the non-shared_modules methods should be declared in a different header in shared-modules. The headers in shared-bindings are meant to capture what methods the bindings depend directly on. common_hal methods need to be implemented by each port while shard_modules don't.

I totally agree that the .h file should be with the .c file. I followed the changes for urandom to random. shared-bindings\random is like what i did, but i will make shared-bindings\struct case as you like.

mrmcwethy · 2017-10-02T23:17:02Z

I made the requested changes, built and tested on a metro board.

stale

tannewt · 2017-10-03T18:42:12Z

Is this ready for another look?

mrmcwethy · 2017-10-03T19:02:58Z

Yes, it is ready. Please review

tannewt

Thanks for more work on this!

tannewt · 2017-10-03T19:11:14Z

Lets raise an error here just like CPython

Not sure what python exception you want to raise: ValueError?

Ideally struct.error like CPython.

>>> import struct >>> b = bytearray(8) >>> struct.error <class 'struct.error'> >>> struct.pack_into("<H", b, 0, 10, 10) Traceback (most recent call last): File "<stdin>", line 1, in <module> struct.error: pack_into expected 1 items for packing (got 2)

You may need to add it similar to: https://github.com/adafruit/circuitpython/blob/master/py/objexcept.c#L200

It looks like its a singleton object on struct.error as well. So its weird. Its ok to skip for now if you like.

I have been struggling with adding struct.error. I have created a new exception "struct.error" in objexcept.c, but you cannot handle it with try/except, so its not much good. I try to add a property to return this exception, but I cannot figure it out. Honestly, unless you want help me figure out how to add an object attribute containing an except, I think we should create an exception called StructError and emit that for our error cases.

Yeah, I get its tough. How about just using an existing error like RuntimeError for now. I can go back and take a look into it later.

tannewt · 2017-10-03T19:22:01Z

Lets raise a typecode error here if the fmt is 'O' or 'S' because they are non-standard.

Official ones are here: https://docs.python.org/3/library/struct.html#format-characters

ValueError?

I added an internal method to exclude 'S' and 'O'. Called in three different shared module method.

tannewt · 2017-10-03T19:25:02Z

I know its kinda weird but the shared_modules methods should be in the header in shared-bindings because they are whats used by the code in shared-bindings. This header should only have the stuff used locally by shared-modules.

Sorry about being so thick on this. I think i get it now.

tannewt · 2017-10-03T20:38:40Z

Hey, @mrmcwethy I just realized this should be based on master. The rename is a big enough change that we should hold off on it until 3.x.

mrmcwethy · 2017-10-03T22:16:58Z

I agree, the changes to circuitpython should not be part of 2.x series, should be in 3.0 beta or later. i was told to make changes on the 2.x branch. Is it easy to switch branches?

mrmcwethy · 2017-10-04T13:28:25Z

Ok, i made another round of changes. not sure what to do about master vs 2.x

tannewt · 2017-10-04T23:58:56Z

I haven't looked at everything yet. Here are instructions for changing the branch. https://github.com/blog/2224-change-the-base-branch-of-a-pull-request

mrmcwethy · 2017-10-05T13:25:51Z

I changed the base to master and it looks like there is only one conflict! In the atmel-samd/mpconfigport.h you had removed most of the shared-binding. In the case of struct_module, there are no processor dependencies and as a result, i think we allow the struct_module to be included.

tannewt

Whats the merge conflict? I don't see it here.

tannewt · 2017-10-09T19:51:03Z

Ideally struct.error like CPython.

>>> import struct >>> b = bytearray(8) >>> struct.error <class 'struct.error'> >>> struct.pack_into("<H", b, 0, 10, 10) Traceback (most recent call last): File "<stdin>", line 1, in <module> struct.error: pack_into expected 1 items for packing (got 2)

You may need to add it similar to: https://github.com/adafruit/circuitpython/blob/master/py/objexcept.c#L200

tannewt · 2017-10-09T20:01:22Z

It looks like its a singleton object on struct.error as well. So its weird. Its ok to skip for now if you like.

tannewt · 2017-10-10T17:57:37Z

Looks like the merge conflict is that in master I've disabled a bunch on the modules. You should be able to add struct to the short list of enabled ones and it'll work.

tannewt · 2017-10-12T17:24:07Z

I thought I fixed the merge conflict but still see errors. Would you mind rebasing this locally? Thanks!

mrmcwethy · 2017-10-12T17:33:00Z

@tannewt may take a few days to get to this due to illness.

tannewt · 2017-10-12T17:37:23Z

No worries! Feel better @mrmcwethy!

…ruct

…module

mrmcwethy · 2017-10-16T16:58:53Z

The rebase adafruit/master looks good. i still need to address the exception review comment.

mrmcwethy · 2017-10-18T18:05:04Z

I think i have made required changes for now.

tannewt

Thanks for your help with this! I've approved, please file a newer issue to correct the exception type. Thanks!

tannewt self-requested a review October 2, 2017 22:11

tannewt previously requested changes Oct 2, 2017

View reviewed changes

tannewt requested changes Oct 3, 2017

View reviewed changes

mrmcwethy changed the base branch from 2.x to master October 5, 2017 13:19

tannewt reviewed Oct 9, 2017

View reviewed changes

mrmcwethy added 6 commits October 16, 2017 08:54

added struct module to shared-bindings and shared-module. removed ust…

f259dc7

…ruct

for esp8266 as well

cdf09cb

update documentation. moved some code from shared-bindings to shared-…

fa2d757

…module

Updates request by reviewer

43a6978

added a include to shared-binding/struct just because it seems right

2f2259f

moved declatations to shared_bindings. excludes 'S' and 'O'

269fd52

mrmcwethy force-pushed the struct2.x branch from 57fed83 to 269fd52 Compare October 16, 2017 16:56

mrmcwethy added 3 commits October 16, 2017 10:27

put back a commit mpconfigport.h

3515a7d

change raise ValueError to raise RuntimeError

43d403b

Fixed one last reference that caused compile error

4965ada

tannewt approved these changes Oct 18, 2017

View reviewed changes

tannewt merged commit b41cbe9 into adafruit:master Oct 18, 2017

Conversation

mrmcwethy commented Oct 2, 2017

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrmcwethy commented Oct 2, 2017

Uh oh!

tannewt commented Oct 3, 2017

Uh oh!

mrmcwethy commented Oct 3, 2017

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrmcwethy Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt commented Oct 3, 2017

Uh oh!

mrmcwethy commented Oct 3, 2017

Uh oh!

mrmcwethy commented Oct 4, 2017

Uh oh!

tannewt commented Oct 4, 2017

Uh oh!

mrmcwethy commented Oct 5, 2017

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrmcwethy Oct 4, 2017 •

edited

Loading