Fixed a few parsing issues:

[kueblc]

zero-padding is actually the return code from the device where 0 = success
Added _returnCode, _crc to MessageParser
(reordered to match order in memory)
(crc is not actually checked here, but it could be!)
More restrictive length checks, taking into account _returnCode and _crc
_commandByte is a uint32
Fill _leftOver with, well, leftovers
Reordered parse logic to match order in memory
(this also allows us to extract more data in case of a suffix mismatch)
suffix indexed relative to _payloadSize, relevant in the case of leftovers
(which happens when two messages are sent in one)

These changes were made with the best attempt to adhere to the existing coding style
I would personally make parse and encode both static functions
We could check the crc value for data integrity
If _leftOver is populated, better try decoding that too

A more drastic overhaul can be found:
https://github.com/kueblc/mocktuyacloud/blob/master/lib/lan-frame.js
But no attempt has been made to maintain the previous coding style
This adaptation was made to integrate seamlessly into the existing tuyapi project

[kueblc]

The Travis failure is due to a bug in the stub device implementation. Since a return code is only present in a message from the device to the client, the same parsing and encoding logic shouldn't be used in both device and client.

[kueblc]

I can add logic to accommodate the bug but it would be better to address the bug instead.

[codetheweb]

Thanks for the contributions @kueblc. To answer your questions from the other PRs, the only reason they're not static functions is the contributor who originally wrote them decided to put them in classes. I'm totally ok with making them static functions; make any changes you think necessary.

Is it possible to make a new PR with all your changes in a single place? Just makes it easier for me to see what's changed.

[kueblc]

Sure, I can combine changes into a single PR if you'd prefer. The practice of isolating changes to individual branches is so that the project maintainer can independently evaluate the impact and decide to pick and choose which changes to merge.

For instance, this PR, while code correct, is failing checks. It might be that you want to hold off on merge this piece until a PR is made for tuyapi/stub. Each of these PRs were coded to be independently and cleanly merged.

If you're onboard with the direction I'm going, I'll gladly merge these together and keep it going. I wanted to see what, if any, of my additions fit your vision for the project before I invested much.

Moving forward, perhaps we can create a CONTRIBUTING document that establishes a policy of coding style and workflow, dictating how changes are to be made and submitted; and project guidelines to define the scope of the project along with acceptable and unacceptable methods (ie, no extracted keys).

I've got a number of breaking and non-breaking changes in mind, and of course I'll be putting forth non-breaking changes first. I reference MockTuyaCloud as it encompasses a lot of the changes I wanted to make, but it does so in a breaking fashion and with my own coding style, so that's why I'm now trying to backport these changes in a non-breaking way.

If you have any thoughts on approach, please let me know and I'd be happy to make any coming changes with that in mind. I've so far remained fairly conservative in changes but like I said before, see MockTuyaCloud for an idea of my coding style and direction.

[codetheweb]

Got it, thanks for the detailed response.

It was a little difficult to see that each PR could be merged separately at first glance, but now I see each PR is based on a different branch. If it's ok with you, I'll close all of your currently open PRs and then you can make a single PR with all changes to the cipher and parsing functions when you're ready.

I'm aware tuyapi/stub is far from perfect; I've been running into some issues over the last few days. Still working on it. (While running tests and working with TuyAPI, it appears it may possible to overwrite data sent from the socket. It may be necessary to queue ._send() calls, although I really don't want to do that.)

As far as breaking changes go, I think the current interface (although not perfect) is pretty good. Breaking the cipher and parsing functions/classes is fine, but I'd prefer to keep the user-facing interface largely the same; unless you have a really cool idea for X feature.

Also, just as a heads up: I'm doing a hackathon this weekend; so I'll be unavailable until late Monday/Tuesday.

[kueblc]

Sure go ahead and close and I'll put these together in a new one. Would you like to merge the more-tests branch now or should that be merged in with the cipher and parser changes as well? Should we be contributing any future changes on the same branch moving forward?

Another reason to push and pull often is to keep all contributors up to date on what's changing to minimize merge conflicts. If no one else is working on these parts right now, I'd like to do a bit more liberal clean up. I can maintain code style and stick to non-breaking changes.

I'd be happy to help debug tuyapi/stub next if you'd like any help. I created something like that, a fake device, but for testing the cloud functions. I don't think you should be losing packets, or maybe something just needs to be flushed.

As breaking changes go, the only thing that bugs me is the interface for .set(), I think it adds an unnecessary layer of abstraction that's more verbose than the last. Basically I think it should just pass the set of changes directly, as if multiple is the default.

So instead of
.set({dps: 2, set: false})
it's
.set({2: false})

I haven't had any issues setting multiple dp values at once, and in many cases I'm finding I need to, such as in the use case of power strips and RGBW light bulbs.

It's much easier, and cleaner IMHO, to turn the bulb on at half brightness with
.set({1: true, 2: 'white', 3: 127})
rather than
.set({set: true})
.set({dps: 2, set: 'white'})
.set({dps: 3, set: 127})

It also avoids any possible delay between these adjustments taking place. You can go directly from off to any color, brightness or scene, or any combination between them.

You can also use this to easily make a nice little interface for a given device
const setBulb = (power, mode, brightness, temp, color) => bulb.set({1: power, 2: mode, 3: brightness, 4: temp, 5: color})
or with a few more lines you could define an interface based off a schema.

There's a lot more that could be said here, just trying to give you an impression and see what you think.

No worries if you don't get to this right away, open source is powered by volunteered time and we appreciate every minute of it! Good luck at your Hackathon!

[codetheweb]

I went ahead and merged the more-tests PR.

AFAIK, you and I are the only people actively working on this. Feel free to make a separate PR for each major change or a single gigantic PR, whatever you prefer.

I see what you mean with .set(), it is a bit cumbersome. I'm totally open to changing it to something like that but it'd be great if it was backwards-compatible somehow.

[kueblc]

Hey @codetheweb, hope the hackathon went well!

AFAIK, you and I are the only people actively working on this. Feel free to make a separate PR for each major change or a single gigantic PR, whatever you prefer.

I was hesitant to put all revisions in a single branch/PR, in case you disagree with any of the changes I wanted to make it easy to pick and choose. Also concerned about project stability, could you create a development branch to stage changes from PRs before merging to master?

My thought is that we'll collaborate in the development branch, and when we're all set and good with the changes, increment the version and merge into master for release.

If it's too much to break the changes down into individual branches/PRs, I propose the policy of merging those individual feature branches locally into username/username-dev branches, which are then periodically merged into origin/development.

I see what you mean with .set(), it is a bit cumbersome. I'm totally open to changing it to something like that but it'd be great if it was backwards-compatible somehow.

How about a new method, to the effect of .setDirect(), that's called by .set() under the hood, and deprecating .set() for 5.0.0?

We could also play with other layers of abstraction, namely a very thin wrapper that translates fields to dp values a la schema, ie .setSchema({power: true, mode: 'white', brightness: 50}). This could be inferred from get or defined at instantiation time ie new TuyaDevice({id: ..., key: ..., schema: {power: 1, mode: 2, brightness: 3 ...}}).

Another thought, since we're receiving updates in real time from the device, we can maintain our own dps locally, so calls to get don't necessarily need to make an extra call to the device unless dps hasn't been initialized yet. At that, perhaps we can make a call to get as part of device instantiation such that explicit calls to get aren't necessary. The dps would be fetched in constructor, ready event would be fired when dps values are loaded, then real time updates from the device would keep our local dps current.

Some things to think about. I'll focus on non-breaking revisions for now and we can talk more about API as we move forward. It'll be more clear if I can show some examples of what I'm talking about.

[codetheweb]

Thanks, the event was quite a bit of fun.

Also concerned about project stability, could you create a development branch to stage changes from PRs before merging to master?

Good idea, just created one. Didn't feel the need before as I generally published a new version soon after committing to master, but it definitely makes sense with more active contributors. 😃

If it's too much to break the changes down into individual branches/PRs, I propose the policy of merging those individual feature branches locally into username/username-dev branches, which are then periodically merged into origin/development.

I'm fine with breaking up changes/PRs into granular parts (it seems logical after you explained it); not sure if it makes sense to have a different branch for each contributor if contributors can just fork this and make changes on their forked version.

Another thought, since we're receiving updates in real time from the device, we can maintain our own dps locally, so calls to get don't necessarily need to make an extra call to the device unless dps hasn't been initialized yet.

I was actually thinking about including something similar in v4.0, but couldn't figure out an elegant way to let the user decide if they want to fire a get request or just get the data returned from the last event. Completely relying on the device to send updates may result in an un-synchronized state.

[kueblc]

Thanks, the event was quite a bit of fun.

I haven't been to one in a while (not as accessible once you're out of school) but for sure I had fun. Anything interesting projects to share? Tuya hacking? 😛

Good idea, just created one. Didn't feel the need before as I generally published a new version soon after committing to master, but it definitely makes sense with more active contributors. smiley

Awesome, I'll make PRs to this branch moving forward, thanks!

I'm fine with breaking up changes/PRs into granular parts (it seems logical after you explained it); not sure if it makes sense to have a different branch for each contributor if contributors can just fork this and make changes on their forked version.

Sounds good.

I was actually thinking about including something similar in v4.0, but couldn't figure out an elegant way to let the user decide if they want to fire a get request or just get the data returned from the last event. Completely relying on the device to send updates may result in an un-synchronized state.

Maybe a logical separation between update which actively gets state from the device, and get which returns the corresponding value from local cache? I rewrote a lot of functionality a couple months ago for my own project, I'll try to provide some examples once I refresh myself with the changes and why I made them.

[codetheweb]

It's not that interesting or complex, but a friend and I made this. It's a webapp for party games that require players to "buzz in". First person that joins a room is the host, everyone who joins after is a player. Real time communications done with Socket.IO.

Anyways, adding an update function sounds good.

Also, didn't mention it above but I like your idea of an automatic schema for .set() and .get(), I'll look into that.