-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add cctalk parsers #1342
Add cctalk parsers #1342
Conversation
This is by the way 100% tested on real hardware in production its based on work: Only made compatible because i use some extra dependencies because of the ES7 and will switch to debug-extra https://github.com/direktspeed/debug-extra |
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
=========================================
+ Coverage 78.97% 79.58% +0.6%
=========================================
Files 20 20
Lines 899 862 -37
Branches 164 162 -2
=========================================
- Hits 710 686 -24
+ Misses 189 176 -13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint failure on Travis is weird, I'll look into it. This just needs at least a basic unit test (eg, this binary in gets these data events) so people not familiar with it's operations can ensure they don't break it and it's good to merge =)
lib/parsers/cctalk.js
Outdated
} | ||
_transform(buffer, _, cb) { | ||
debug('parser set')(this.buffer, this.cursor); | ||
debug('parser set')(this.buffer.toString('hex'), buffer.buffer, this.cursor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd either push this into a conditional debug.enabled
or use a custom formatter so toString
doesn't get called when not debugging.
You can now rebase and travis will pass linting |
…ode-serialport into add_cctalk_parsers
@reconbot can you please contribute to the tests? we need to test the following Arrays write [2,0,1,254,217] // its length 5 write [2,2,1,254,1,1,217] // Length 7 |
@reconbot and please look into the current travis error i don't know any more whats going on as i don't know why they deprecated that because it makes sense to set some arrays at array position. |
The |
@reconbot yes the tests are not done when i remove them and do it localy i get totall mess :)
|
@@ -76,6 +76,7 @@ parser.on('data', console.log); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some docs up here ☝️
@@ -71,7 +71,7 @@ | |||
"chai": "^4.0.2", | |||
"chai-subset": "^1.5.0", | |||
"conventional-changelog-cli": "^1.3.2", | |||
"eslint": "^4.5.0", | |||
"eslint": "^4.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so weird, we totally have this in master
lib/parsers/cctalk.js
Outdated
} | ||
_transform(buffer, _, cb) { | ||
// TODO: Better Faster es7 no supported by node 4 | ||
const array = Array.prototype.slice.call(buffer, 0); // [...buffer]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.from(buffer)
but this seems weird that casting from a buffer to an array to a Uint8Array
is the right way forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because buffers don't support the needed methods they are really static we need a event chain so i choosed to go over to array
These tests don't pass but its a start 'use strict';
/* eslint-disable no-new */
const Buffer = require('safe-buffer').Buffer;
const sinon = require('sinon');
const CCTalkParser = require('../lib/parsers/cctalk');
describe('CCTalkParser', () => {
it('emits data for a default length message', () => {
const data = Buffer.from([2, 0, 1, 254, 217]);
const spy = sinon.spy();
const parser = new CCTalkParser();
parser.on('data', spy);
parser.write(data);
assert.equal(spy.callCount, 1);
assert.deepEqual(spy.callArgWith(Buffer.from([1, 0, 2, 0, 217])));
});
it('emits data for a 7 byte length message', () => {
const parser = new CCTalkParser();
const spy = sinon.spy();
parser.on('data', spy);
parser.write([2, 2, 1, 254, 1, 1, 217]);
assert.equal(spy.callCount, 1);
assert.deepEqual(spy.getCall(0).args[0], Buffer.from([1, 0, 2, 0, 217]));
});
}); |
@reconbot i think you got it wrong because the answer should come from the device not from the parser the parser will simply emit the complet message written to it |
[2, 0, 1, 254, 217] writen to the mockup device should let the mock up device write [1, 0, 2, 0, 217] for the parser its enought if he emits 2 commands from this write [2, 0, 1, 254, 217,2,2,1,254,1,1,217] first is 5 length secund is 7 length |
We're testing the wrong thing then, the parser is a stream, the stream has
a behavior, and we need to ensure when we write to it, we get the right
behavior. Nothing more.
…On Sun, Sep 24, 2017, 3:32 PM Frank Lemanschik ***@***.***> wrote:
@reconbot <https://github.com/reconbot> i think you got it wrong because
the answer should come from the device not from the parser the parser will
simply emit the complet message written to it
so when we don't use your magic mockup framework to emit on that write the
correct answer it can't work
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1342 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABlbjoeVSVD_Rj2WWI-T5V3y8D7SxCEks5slq4_gaJpZM4Pghel>
.
|
@reconbot for the parser its enought if he emits 2 commands from this write [2, 0, 1, 254, 217,2,2,1,254,1,1,217] |
Perfect, and |
@reconbot but i don't understand it i will write a more good test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Done! LGTM
@reconbot anything left undone ? it looks good to me? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Done
Yeah this does look good. =) Sorry got busy with a big launch at work tomorrow. |
released with [email protected] 🎉 |
this adds the discussed cctalk parser
#1240