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

Functionality required for ZCL support #13

Merged
merged 7 commits into from
Aug 20, 2015
Merged

Functionality required for ZCL support #13

merged 7 commits into from
Aug 20, 2015

Conversation

rtzoeller
Copy link
Contributor

Added a FieldArray type which supports a variable number of another structure being embedded inside of it. Complex usage of this datatype necessitated a TypeField class which acts as a hybrid between a DispatchField/DispatchTarget and a LengthProvider, allowing for the length of a field to be given by a dictionary mapping. An example of why this is necessary can be seen here.

Also added support for 24, 40, 48 and 56-bit integers.

@posborne
Copy link
Contributor

For unit tests, it would be good to get basic coverage on the new lines not covered: https://coveralls.io/builds/3326418/source?filename=suitcase%2Ffields.py.

You can get similar coverage results locally by doing:

nosetests --with-coverage --cover-html . && firefox cover/index.html

@rtzoeller
Copy link
Contributor Author

The tests should cover everything now; let me know if you see something I missed.

variable number. The variable nature of these fields make a DispatchField
and DispatchTarget combination unsuitable; instead a FieldArray may be
used to pack/unpack these fields to/from a list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since TypeField and FieldArray are a little tricky, I'm wondering if we can include a small example in the docstring. I find that this is extremely valuable when reading documentation. Here's what I came up with for FieldArray.

class PascalString16(Structure):
    length = LengthField(UBInt16())
    value = Payload(length)

class FamousQuoteRecord(Structure):
    author = PascalString16()
    quotes = FieldArray(PascalString16)  # variable number of separate quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your example, but I'm having a hard time getting structures to embed properly (regardless of my pull request). Can you let me know if I'm doing something inherently incorrect in this test?

from suitcase.fields import LengthField, UBInt16, Payload
from suitcase.structure import Structure


class PascalString16(Structure):
    length = LengthField(UBInt16())
    value = Payload(length)


class FamousQuoteRecord(Structure):
    length = LengthField(UBInt16())
    value = Payload(length)

    author = PascalString16()


m = FamousQuoteRecord()
m.value = b"John Doe"
m.author.value = b"Jane Doe"

print(m.author.pack())  # Prints b"\x00\x08Jane Doe"
print(m.pack())  # Should print b"\x00\x08John Doe\x00\x08Jane Doe", but only prints b"\x00\x08John Doe"

Copy link
Contributor

Choose a reason for hiding this comment

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

Come to think of it, I have never embedded a structure in this way on a project, so it is possible that this just doesn't work right now (and probably should). I think that non-fields that get added will just be ignored, so we would need to do something like the following to get just one.

class FamousQuoteRecord(Structure):
    length = LengthField(UBInt16())
    value = Payload(length)
    author = SubstructureField(PascalString16())

Or, maybe we could do something like...

author = PascalString16.to_field()

Or

PascalStringField = make_structure_field(PascalString16)

# ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fitting with the FieldArray format, I like this pattern

author = SubstructureField(PascalString16)

which omits the object instantiation. I'll work on writing up support for that today, unless you are in favor of one of the other syntaxes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@posborne
Copy link
Contributor

Coverage looks good. Let's get an example added for each of the two new fields in the documentation and after that I think this is looking good to merge.

@rtzoeller
Copy link
Contributor Author

I still need to add an example for the TypeField, however the SubstructureField is implemented and FieldArray was given an example.

I was not able to find a way to support Greedy substructures, so that might be something to look into. I'm not a huge fan of the special case I needed to add to Structure's packing mechanism, but it looked like the cleanest solution. If you can think of a way that doesn't require a special case that solution might be preferable over mine.

@rtzoeller
Copy link
Contributor Author

The permissions on some files got clobbered at some point and Travis CI didn't like it, so I had to rebase a few things to fix the history. Everything should be fine now, other than Coveralls pointing to some deleted commits that I made while trying to debug, as the problem didn't manifest itself locally.

@posborne posborne merged commit f6532e2 into digidotcom:master Aug 20, 2015
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

2 participants