search

ZoetropeLabs / AS5048A-Arduino

A simple SPI library to interface with Austria Microsystem's AS5048A angle sensor
MIT License
39 stars 28 forks source link

using SPISettings #2

Closed Adrianotiger closed 8 years ago

Adrianotiger commented 8 years ago

Hi Ben, I added the SPISettings to the class, so it is possible to add more SPI devices using this library. Replaced also the defines with const... Arduino Reference wrote it is better :P (https://www.arduino.cc/en/Reference/Define) The clock is set to 1MHz (on the AMS5047 it is possible to set it up to 10MHz, don't know how it is on the 5048).

This is my first pull request, hope everything is ok. Thank you again for this AMS library! Adriano

benhowes commented 8 years ago

It's certainly looking great for a first PR!

A few small points:

  1. I've added a comment on a few lines which are just commented out, but should be removed to keep things tidy
  2. You've removed support for pre 1.0 Arduino - was this intentional?
  3. Have you been able to test your code with more than one device?

It might be handy to also add a quick note or snippet to the readme saying about multiple device support?

Thanks very much for taking the time to contribute back!

Adrianotiger commented 8 years ago

Can't you change the code or select the changes on a pull request? I made some changes but I was not sure that you want every changes I made, so I commented them out.

  1. I commented them out, because you used another clock than the one I used on this library.
  2. Yes was intentional... Was thinking that Arduino 0.22 is too old and nobody use the old IDE anymore :P
  3. Yes I was able and I am using this code for my project (using 2 SPI devices) - next week I can make some other tests and say if it is working at 100% :)

The open Issue was not to say that something was wrong... I opened only to say that there are some improvement possibilities, hehehe The library is great and thank to this library I was able to test the AMS without any heavy work :D I changed some lines so that it was easy to implement it on my project. Feel free to change or remove the pull request. As said, I am new to GitHub and is not so easy like the TFS application.

benhowes commented 8 years ago

Nope, I only get the option to merge or not merge your branch back in to the master branch. I can't edit your branch either, since that's owned and controlled by you :).

  1. That's fine, I think 1MHz is sensible - if you could just remove those lines I'll be able to cleanly merge your code back in.
  2. That's also fine - this module was originally made quite some time ago
  3. That's great news

You've done the right thing, opening a ticket is the right way to get our attention. I'll close it off when I merge in your changes.

Adrianotiger commented 8 years ago

Removed :relieved: Does I need to make a new pull request? I will test this library next week, I will give you the :ok: once I fully tested it!

benhowes commented 8 years ago

That's great! your commits on the same branch all go in. Ping me when you're happy that it's all working!

benhowes commented 8 years ago

@Adrianotiger Did you have any luck with further testing of this? Keen to merge it in :)

Adrianotiger commented 8 years ago

Hi Ben, sorry for the late answer. Yes I didn't had any problem with other SPI settings at the same time. Thank you again for your code!