Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Construct related updates #19

Closed
wants to merge 10 commits into from
Closed

Construct related updates #19

wants to merge 10 commits into from

Conversation

arekbulski
Copy link

@arekbulski arekbulski commented Apr 3, 2018

For reasons I would rather skip, I could not update previous PR so I am withdrawing previous one and submitting a new one with both previous and new updates. The code presented should work with Construct version 41 (homeassisstant pinned) and 43 (most uptodate).

There are few things that need to be adjusted for the code to run, but you will be able to find those by looking at comments.

@arekbulski arekbulski mentioned this pull request Apr 3, 2018
@rytilahti
Copy link
Owner

Hi Arkadiusz and thanks for the updated PR! I will try and look into those parts needing a review soon (most of the protocol itself is not currently used that much, and merely left there for documentary reasons), and merge after adapting as necessary.

@hcoohb
Copy link

hcoohb commented Apr 4, 2018

Thanks a lot @arekbulski and @rytilahti for spending some time to update this code!

@hcoohb
Copy link

hcoohb commented May 26, 2018

Hi @rytilahti , Have you had any chance to check the code to be able to merge to master ?
Thanks,

@rytilahti
Copy link
Owner

I tested it but there were some errors, and unfortunately I haven't had time to look into fixing those, I'm afraid.

@arekbulski
Copy link
Author

I am not even faimiliar with this project, so I cant help either.

@rytilahti
Copy link
Owner

So I took a look into this and to me it seems that when using EmbeddedSwitch it is now necessary to pass all the possible fields from each possible case for build(), which is not so easily doable.

@arekbulski am I doing something stupidly wrong here, or is this expected?

$ cat test.py && python test.py
from construct import EmbeddedSwitch, Struct, Const, Byte, this

x = EmbeddedSwitch(
  Struct(
    "type" / Byte,
  ),
  this.type,
  {
    0: Struct("first_case" / Byte),
    1: Struct("second_case" / Byte)
  }
)


print("== type 1==\n%s" % x.parse(x.build({'type': 0x00, 'first_case': 0x11, 'second_case': 0x22})))
print("== type 2 ==\n%s" % x.parse(x.build({'type': 0x01, 'first_case': 0x11, 'second_case': 0x22})))

print("failing case")
print(x.parse(x.build({'type': 0x00, 'first_case': 0x03})))

Output:

== type 1==
Container: 
    type = 0
    first_case = 17
    second_case = None
== type 2 ==
Container: 
    type = 1
    first_case = None
    second_case = 34
failing case
Traceback (most recent call last):
  File "test.py", line 19, in <module>
    print(x.parse(x.build({'type': 0x00, 'first_case': 0x03})))
          │       └ <Struct>
          └ <Struct>
  File "/home/hass/venv/lib/python3.6/site-packages/construct/core.py", line 334, in build
    self.build_stream(obj, stream, **contextkw)
    │                 │    │         └ {}
    │                 │    └ <_io.BytesIO object at 0x7f750e036fc0>
    │                 └ {'type': 0, 'first_case': 3}
    └ <Struct>
  File "/home/hass/venv/lib/python3.6/site-packages/construct/core.py", line 344, in build_stream
    self._build(obj, stream, context, "(building)")
    │           │    │       └ Container()
    │           │    └ <_io.BytesIO object at 0x7f750e036fc0>
    │           └ {'type': 0, 'first_case': 3}
    └ <Struct>
  File "/home/hass/venv/lib/python3.6/site-packages/construct/core.py", line 2082, in _build
    subobj = obj[sc.name] # raises KeyError
    │        │   └ <Renamed second_case <IfThenElse>>
    │        └ {'type': 0, 'first_case': 3}
    └ 3
KeyError: 'second_case'

@arekbulski
Copy link
Author

I clearly see what you mean. Let me investigate it and get back to you.

@hcoohb
Copy link

hcoohb commented Jul 18, 2018

@arekbulski, Did you manage to find out why this was failing?
Thx

@arekbulski
Copy link
Author

I am sorry, but am too busy to maintain Construct anymore. There are few open issues remaining, and I will address them in some foreseeable future, and then wont be maintaining it (extending it) anymore.

@rytilahti
Copy link
Owner

@arekbulski fair enough, and thanks for all your work on construct! Instead of SymmetricMapping, do you have a recommendation how should I handle this case without EmbeddedSwitch? Just having a regular Switch?

Btw, github has nowadays also dependents information, iirc you mentioned at some point not knowing where construct is being used, so that's probably interesting for you https://github.com/construct/construct/network/dependents :-)

@arekbulski
Copy link
Author

A regular switch would solve embedding issues, sure.
SymmetricMapping was from what I can remember just renamed to Mapping, not changed.

@rytilahti
Copy link
Owner

rytilahti commented Feb 26, 2019

Superceded by #19, all thanks go to @arekbulski!

@rytilahti rytilahti closed this Feb 26, 2019
@arekbulski
Copy link
Author

To all interested parties: I am no longer working on (supporting) Construct. There will be no further updates to it (except one or two that are already PRed and RTM), as far as I am planning it. That said, I maintain what I said that there is no promise of backward compatibility.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants