fivdi / i2c-bus

I2C serial bus access with Node.js
MIT License
348 stars 57 forks source link

Windows Support #20

Closed munyirik closed 8 years ago

munyirik commented 8 years ago

I started working on Windows support for i2c-bus and this is my initial PR. The need for this came about when I started working on a project that requires i2c and runs on Windows IoT Core. In the implementation, what I did was try to match the i2c-bus functions with the methods exposed by the i2cDevice class on Windows.

There are still functions that are not implemented yet but I wanted to get this out early to get feedback as soon as possible. Note, there might be some functions that stay 'not implemented' since I can only use what i2cDevice exposes. It does however provide basic functionality to do basic read/write which was sufficient for my project. I also added an example on how to use it to read temperature from a MCP9808 temperature sensor.

Building it does require a change I made to gyp that is not yet in the node branch. So until that change is merged to the node Github repo, building will require the following changes to gyp in your node installation: 1.Replace \node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\generator\msvs.py with msvs.py.txt 2.Replace \node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\MSVSSettings.py with MSVSSettings.py.txt [Edit] be sure to remove the .txt extensions from the files above

fivdi commented 8 years ago

I started working on Windows support for i2c-bus and this is my initial PR.

Windows support for i2c-bus would be really good.

The need for this came about when I started working on a project that requires i2c and runs on Windows IoT Core.

I hope it's not an impolite question but what project are you working on? Is it an OSS project?

There are still functions that are not implemented yet but I wanted to get this out early to get feedback as soon as possible.

I took a quick look at the PR and added some comments. I'll take a longer look soon. So far some of the sync functions have been implemented. Will async functions also be implemented?

Note, there might be some functions that stay 'not implemented' since I can only use what i2cDevice exposes.

Do you have an idea what functions might stay not implemented?

Building it does require a change I made to gyp that is not yet in the node branch. So until that change is merged to the node Github repo, building will require the following changes to gyp in your node installation: 1.Replace \node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\generator\msvs.py with msvs.py.txt 2.Replace \node_modules\npm\node_modules\node-gyp\gyp\pylib\gyp\MSVSSettings.py with MSVSSettings.py.txt

I don't have a Windows machine to build it. I'll see if I can arrange one.

munyirik commented 8 years ago

Windows support for i2c-bus would be really good.

Awesome!

I hope it's not an impolite question but what project are you working on? Is it an OSS project?

Not impolite at all:-) !. Yes it's OSS. I'm working on getting this hexapod working on Windows with Node.js.

I took a quick look at the PR and added some comments. I'll take a longer look soon. So far some of the sync functions have been implemented. Will async functions also be implemented?

Yes. I'm planning to implement those as well.

Do you have an idea what functions might stay not implemented?

An example might be i2cFuncs. But I'll do some digging to see if there's a way to get the same functionality without using i2cDevice. Same goes for other functions that fall in this category.

I don't have a Windows machine to build it. I'll see if I can arrange one.

That would be great. Let me know if you have any more questions.

fivdi commented 8 years ago

I'm unfamiliar with Windows IoT and this is likely to be visible in some of the comments and questions this time. Sorry about that.

On Linux, the i2c-bus methods can be regarded as atomic transactions. This makes it possible to write code that calls multiple async operations concurrently and the operations will not interfere with each other. An example of this can be seen in https://github.com/fivdi/i2c-bus/blob/master/example/two-devices.js. This example accesses two different devices at the same time asynchronously. In the background the operating system will ensure that the two different devices are actually accessed sequentially. I'd assume that the same is possible with Windows. Is this assumption correct?

Some of the C++ code uses try/catch blocks for exception handling, for example, here. There are also sections of code without exception handling, for example, here. Does the lack of try/catch blocks here mean that no exceptions can be thrown?

It looks like it will take a while before I'm in possession of a Windows 10 machine which is required for Windows IoT development if I'm not mistaken. On the other hand, the PR is non-intrusive and doesn't break any existing code. This means the PR could be merged without me actually knowing whether or not it functioned correctly.

If the PR is merged do you intend maintaining the merged code in the future or is this something you don't want to or would prefer not to do?

munyirik commented 8 years ago

I believe your assumption is correct about the sequential access but let me confirm that and get back you you. As for the try/catch blocks, they are actually not needed based on the documentation and examples I've look at so that was overkill. I'll take them out. I do plan on maintaining the merged code at least to fix any issues in the existing code and also to complete the async functionality.

munyirik commented 8 years ago

@fivdi i2cDevice methods are also atomic. The driver does all the necessary synchronization.

fivdi commented 8 years ago

@fivdi i2cDevice methods are also atomic. The driver does all the necessary synchronization.

@munyirik excellent :)

munyirik commented 8 years ago

I added the async versions of the initial functions I added. I need to do a little more testing before you merge. Also if you get a chance to try it out that would ease my mind as well:-).

fivdi commented 8 years ago

I need to do a little more testing before you merge.

Yes, please do this. Have you tested with more than one device on the bus at the same time? If not, can you do this also?

Also if you get a chance to try it out that would ease my mind as well:-).

I want to, but I don't have the required hardware at the moment. The way the PR has been structured, it can be merged without breaking anything that's already there. If you have no objections, this is what I'll do. Ok?

munyirik commented 8 years ago

Sounds good! you can merge it now. I'll test using more than one device next week and let you know the results.

fivdi commented 8 years ago

@munyirik do you need a version published on npm?

fivdi commented 8 years ago

@munyirik i2c-bus@1.1.0 was just published on npm

munyirik commented 8 years ago

@fivdi thanks!

munyirik commented 8 years ago

@fivdi I should have a Windows version of example/two-devices.js in the next couple of days (sorry didn't get a chance to it this week).

fivdi commented 8 years ago

@munyirik ok, no problem.

munyirik commented 8 years ago

Sorry for the delay. I just got around to testing with two devices today and I didn't see any issues. I'll add the code for this as well as an example (synchrounous) for the Adafruit SI1145 UV sensor. PR coming shortly...