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

Add cctalk parsers #1342

Merged
merged 28 commits into from
Oct 1, 2017
Merged

Conversation

frank-dspeed
Copy link
Contributor

this adds the discussed cctalk parser
#1240

@frank-dspeed
Copy link
Contributor Author

This is by the way 100% tested on real hardware in production its based on work:
https://github.com/direktspeed/serialport-parsers-cctalk

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
that i created to debug streams and that stuff
its like fs extra extending debug with some stream and promise stuff

@codecov-io
Copy link

codecov-io commented Sep 22, 2017

Codecov Report

Merging #1342 into master will increase coverage by 0.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/parsers/index.js 100% <ø> (ø) ⬆️
lib/parsers/cctalk.js 100% <100%> (ø)
lib/bindings/auto-detect.js 69.23% <0%> (-30.77%) ⬇️
lib/bindings/win32.js
lib/serialport.js 97.16% <0%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4850901...7382fe4. Read the comment docs.

Copy link
Member

@reconbot reconbot left a 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 =)

}
_transform(buffer, _, cb) {
debug('parser set')(this.buffer, this.cursor);
debug('parser set')(this.buffer.toString('hex'), buffer.buffer, this.cursor);
Copy link
Member

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.

@reconbot
Copy link
Member

reconbot commented Sep 22, 2017

You can now rebase and travis will pass linting

@frank-dspeed
Copy link
Contributor Author

@reconbot can you please contribute to the tests?

we need to test the following Arrays

write [2,0,1,254,217] // its length 5
NOTE: the secund byte 0 indicates that the length is 5+0=5
answer [1,0,2,0,217]

write [2,2,1,254,1,1,217] // Length 7
NOTE: the secund 2 indicates that the length is 5+2= 7
answer [1,0,2,0,217]

@frank-dspeed
Copy link
Contributor Author

@reconbot and please look into the current travis error i don't know any more whats going on as
it complains about buffer set wich is not really the same like buffer = that is now deprecated.

i don't know why they deprecated that because it makes sense to set some arrays at array position.

@reconbot
Copy link
Member

The emits data events every 5 bytes doesn't look like it does anything relevant as we're not writing cctalk data. I'm also getting this error locally.

@frank-dspeed
Copy link
Contributor Author

@reconbot yes the tests are not done when i remove them and do it localy i get totall mess :)

     .isOpen
        ✓ is a read only property
        ✓ returns false when the port is created
        ✓ returns false when the port is opening
        1) returns false when the port is opening


  151 passing (237ms)
  11 pending
  1 failing

  1) SerialPort property .isOpen returns false when the port is opening:
     TypeError: Cannot read property 'then' of undefined
      at SerialPort.open (lib/serialport.js:232:46)
      at Context.it (test/serialport.js:238:14)

@@ -76,6 +76,7 @@ parser.on('data', console.log);

Copy link
Member

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",
Copy link
Member

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

}
_transform(buffer, _, cb) {
// TODO: Better Faster es7 no supported by node 4
const array = Array.prototype.slice.call(buffer, 0); // [...buffer];
Copy link
Member

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.

Copy link
Contributor Author

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

@reconbot
Copy link
Member

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]));
  });
});

@frank-dspeed
Copy link
Contributor Author

@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

@frank-dspeed
Copy link
Contributor Author

frank-dspeed commented Sep 24, 2017

[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

@reconbot
Copy link
Member

reconbot commented Sep 24, 2017 via email

@frank-dspeed
Copy link
Contributor Author

@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]
so we need to check if he emits 2 times first time 2, 0, 1, 254, 217 secund time 2,2,1,254,1,1,217 thats it

@reconbot
Copy link
Member

Perfect, and sinon is pretty magical and has good docs.

@frank-dspeed
Copy link
Contributor Author

@reconbot but i don't understand it i will write a more good test

var c = 0
parser.write(Buffer.from([2, 0, 1, 254, 217,2,2,1,254,1,1,217])
parser.on('data',(data)=>{
if (data.length === 5 || data.length === 7) {
  c++
if (c === 2) test passed
} else {
console.log('NOT GOOD')
}

})

Copy link
Contributor Author

@frank-dspeed frank-dspeed left a comment

Choose a reason for hiding this comment

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

All Done! LGTM

@frank-dspeed
Copy link
Contributor Author

@reconbot anything left undone ? it looks good to me?

Copy link
Contributor Author

@frank-dspeed frank-dspeed left a comment

Choose a reason for hiding this comment

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

All Done

@reconbot reconbot merged commit bcb492f into serialport:master Oct 1, 2017
@reconbot
Copy link
Member

reconbot commented Oct 1, 2017

Yeah this does look good. =)

Sorry got busy with a big launch at work tomorrow.

@reconbot
Copy link
Member

reconbot commented Oct 7, 2017

released with [email protected] 🎉

@lock lock bot locked and limited conversation to collaborators Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants