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

Set HDR_SPEED #13

Merged
merged 7 commits into from
Feb 19, 2016
Merged

Set HDR_SPEED #13

merged 7 commits into from
Feb 19, 2016

Conversation

walmik
Copy link
Contributor

@walmik walmik commented Jan 3, 2016

  • Added set Ticks (HDR_SPEED) functionality on File object
  • Added tests for valid & invalid input

To use this function:

file = new Midi.File({
    ticks: 1000
});
file
    .addTrack()
        ...

fs.writeFileSync('test.mid', file.toBytes(), 'binary');

* Added setTicks functionality on File object
* Added tests for valid & invalid input
@walmik
Copy link
Contributor Author

walmik commented Jan 3, 2016

I have 2 implementations for this which I ve put in a jsbin (in case you wanna take a look) http://jsbin.com/wiwuhayuze/1/edit?js,console

}

this.HDR_SPEED = String.fromCharCode((num/256), num%256);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also use buffers and do the same as follows:

var speed = new Buffer(2);
speed.writeInt16BE(num);
this.HDR_SPEED = String.fromCharCode(speed[0], speed[1]);

I did not use this Buffer based approach directly as we are exporting the module for the browser as well. But this seems like the least hacky way so far.

@walmik
Copy link
Contributor Author

walmik commented Jan 30, 2016

Hi @dingram Did you need some more changes to this PR?

@walmik
Copy link
Contributor Author

walmik commented Feb 3, 2016

I ve made the change and moved the input validation but I m not sure what is the range for the valid input. Is it 1 to 65536?

@walmik
Copy link
Contributor Author

walmik commented Feb 3, 2016

I need it for the specific error message.

var c = config || {};
if (c.ticks && typeof c.ticks !== 'number' || c.ticks <= 0 || c.ticks >= (1 << 15)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd probably do:

if (c.ticks) {
  if (typeof c.ticks !== 'number') {
    throw new Error('Ticks per beat must be a number');
  }
  if (c.ticks <=0 || c.ticks >= (1 << 15)) {
    throw new Error('Ticks per beat must be between 1 and 32767');
  }
  this.ticks = Math.floor(c.ticks);
} else {
  this.ticks = 128;
}

in order to have clear error messages for different types of errors... and also to ensure that the number of ticks is an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about nesting ifs but I guess it s fine in this scenario.

@walmik
Copy link
Contributor Author

walmik commented Feb 13, 2016

Hi Dave, do you have any further requirements in this PR?

Apart from that, I had a question. I have a node module that depends on jsdmidgen for converting notes to pitches and rendering to MIDI. Whenever I render, I get a lowered octave from what I set. I am aware that there s some confusion regarding the middle C when it comes to MIDI devices. I d like to know what do you consider as middle C? I m trying to see if there is an issue in my module or is there a difference of opinion when it comes to middle C :)

Thanks!

dingram added a commit that referenced this pull request Feb 19, 2016
Allow number of ticks per beat to be set.
@dingram dingram merged commit e4be814 into dingram:master Feb 19, 2016
@dingram
Copy link
Owner

dingram commented Feb 19, 2016

Middle C is C4 in this library.

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