Closed pvint closed 6 years ago
Looks great, thanks! I like your use of the coding style, but I do have some feedback on the logic so you're docked a week's pay.
If you want to set MODE1
options like ALLCALL
, this will initialize MODE1
without ALLCALL
in PCA9685_initPWM
and then you call _PCA9685_MODE1_opts
to re-initialize MODE1
with ALLCALL
. I think it's better to call _opts
first to change the default (can be a global var) and then use the default in _initPWM
. But if you're just writing a set()
ter without any logic, why not just make it an extern
var in the header file, that the user can assign directly? This saves lines of code, simplifies debugging, and reduces execution time. If you go this route, just follow the model of extern int _PCA9685_DEBUG
but with something like extern unsigned char _PCA9685_MODE1_VALUE
. Then you'd change
// set MODE1 register
ret = _PCA9685_writeI2CReg(fd, addr, _PCA9685_MODE1REG, 1, &mode1val);
to
// set MODE1 register
ret = _PCA9685_writeI2CReg(fd, addr, _PCA9685_MODE1REG, 1, &_PCA9685_MODE1_VALUE);
Of course, you'd remove mode1val
and make sure to initialize _PCA9685_MODE1_VALUE
, just like DEBUG
.
And here are a few things I should have thought to put in the original issue description but I was drinking heavily.
1) update the README.md
with the new entry in VARIABLES
. Some day I'll extract that documentation into something more appropriate..
2) drop a line into the CHANGELOG.md
under [0.6] - unreleased
as Added
Or just have a look at #24 . :smile:
No time over the weekend and busy tonight, but had a quick look at #24 and I think that will do everything I want to do. :)
I'll try to test it later tonight.
PCA9685demo has sample usage for everything (I think).
Thanks again for taking a stab at this for me! Since I've merged in my extern
mode vars, this can be safely closed. Please check out the latest develop
version and let me know what you think!
As per #15
Added:
int PCA9685_MODE1_opts(int fd, unsigned char addr, unsigned char val);
Also added a simple test in PCA9685demo.c which simply sets the value to 0x00 | AUTOINC Tested by setting it to ALLCALL and then clearing it