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

Correctly process APPLICATION/SYSTEM DESCRIBE requests #1226

Merged
merged 1 commit into from Jan 13, 2017

Conversation

@avtolstoy
Copy link
Member

commented Jan 1, 2017

Fixes #1181

NOTE: Device currently receives incorrect DESCRIBE request with flags=0x09 (see https://github.com/spark/firmware/blob/dfc5e68866cdd794d17ab51640ca565ae3a757ab/communication/src/protocol_defs.h#L91). The length of the request also seems to be wrong (judging by #1181), and instead of 9 bytes, the device receives 16 bytes, all extra bytes are 0x09:

Photon:                        41016b7d02b16409 09 09090909090909
Electron DESCRIBE_SYSTEM:      4101305d01b16441 01
Electron DESCRIBE_APPLICATION: 4101305e02b16441 02

The PR accounts for this by assuming that the server actually requests DESCRIBE_ALL (0x03), if describe flags byte >= DESCRIBE_ALL (0x03)


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation (N/A)
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Bugfix

  • [PR #1226] [Fixes #1181] [Photon/P1/Core] Process TCP DESCRIBE properly and return only one response, SYSTEM, APPLICATION, or COMBINED (ALL) describe message. Was sending separate SYSTEM and APPLICATION previously.

@avtolstoy avtolstoy added this to the 0.7.x milestone Jan 1, 2017

@technobly technobly added the bug label Jan 3, 2017

@brycekahle

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

I confirmed the TCP server does not set any flags.

The extra bytes sound like AES padding, which will always be applied to get to multiples of 16 bytes. It appears that whatever handles the AES decryption isn't adjusting the length appropriately.

@technobly technobly modified the milestones: 0.6.1, 0.7.0 Jan 12, 2017

@technobly technobly merged commit a8a17ee into develop Jan 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/describe-response branch Jan 13, 2017

@jlkalberer

This comment has been minimized.

Copy link

commented on c54ecc3 Jan 19, 2017

@avtolstoy - I had issues with this when trying to get functions/variables because of instead of 1 coap message == 1 response, I had to listen for two responses.

This looks like it should fix things but can I expect that the entire description (system + functions + variables) will fit in the COAP message or should I start passing flags to get both?

This comment has been minimized.

Copy link
Member Author

replied Jan 19, 2017

Are we talking UDP or TCP here? Electron receives and processes separate SYSTEM/APPLICATION DESCRIBE requests; I would assume exactly to fit the CoAP message into MTU. With TCP that shouldn't be a problem at all, since TCP is stream based. Photon doesn't receive any flags and defaults to DESCRIBE_ALL. Previously it was responding with two separate messages as described in #1181. There is still a piece of code missing that strips the AES padding, but no-flags + padding should still be correctly handled now.

This comment has been minimized.

Copy link

replied Jan 19, 2017

In my case, I am sending a message over COAP with the local cloud with P1/Photon.

I think my code should handle your changes but the prior behavior was very confusing.

@avtolstoy avtolstoy referenced this pull request May 27, 2017
7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.