getPWM returns random numbers

[dreadlordrider]

I realized that I was getting garbage values and so reverted to a very basic code.
`void setup() {
Serial.begin(9600);
Serial.println("8 channel Servo test!");
pwm.begin();
pwm.setPWMFreq(50);
delay(10);
}

void loop() {
if (Serial.available())
pwm.setPWM(0, 0, Serial.parseInt());

delay(2500);
uint8_t pwmout = pwm.getPWM(0);
Serial.println(pwmout);
}`
I have tried just printing out the value and also typecasting it to char.
I am using a MG996R servo.

I just kept getting values like 4, 6, 19, 224 etc.

[VToegel]

I concur with dreadlordrider, in my test getPWM() method does return only random numbers before the first call to setPWM() and thereafter returns only 0.

[tbolin]

I'm not even sure what getPWM() should be doing. I would expect that it would either return the values for the rising edge and falling edge (two 12 bit values), or the difference between them (one 12 bit value). But getPWM() returns an uint8_t.
The call signature to requestFrom is also weird, at least for the default TwoWire Wired instance.
_i2c->requestFrom((int)_i2caddr, LED0_ON_L + 4 * num, (int)4);
It looks like it is supposed to read 4 bytes from the internal address of pin num, but the method that is called looks like this:

uint8_t TwoWire::requestFrom(int address, int quantity, int sendStop)
{
  return requestFrom((uint8_t)address, (uint8_t)quantity, (uint8_t)sendStop);
}

It appears that the internal address will be interpreted as the number of bytes that should be sent, and the (int)4 parameter indicate that a stop signal should be sent after the transmission is completed.

[dreadlordrider]

I have tried modifying that code in the cpp file based on the PCA9685 datasheet and gotten close. But not there yet.
I have changed requestfrom too.. Realized that it was returning number of bytes.
Right now I am reading the getPWM directly from the respective registers. But somewhere in my gibberish I am getting vague values.

[Michele61]

I modified the get function this way:
/*!

• @brief Gets the PWM output of one of the PCA9685 pins
• @param num One of the PWM output pins, from 0 to 15
• @return requested PWM output value
  */
  uint16_t Adafruit_PWMServoDriver::getPWM(uint8_t channel) {

//_i2c->requestFrom((int)_i2caddr, LED0_ON_L + 4 * num, (int)4);
//return _i2c->read();

byte regAddress = LED0_ON_L + (channel << 2);
_i2c->beginTransmission(_i2caddr);
_i2c->write(regAddress);
_i2c->endTransmission();

_i2c->requestFrom((uint8_t)_i2caddr, (uint8_t)4);
uint16_t phaseBegin = (uint16_t)_i2c->read();
phaseBegin |= (uint16_t)_i2c->read() << 8;
uint16_t phaseEnd = (uint16_t)_i2c->read();
phaseEnd |= (uint16_t)_i2c->read() << 8;
uint16_t retVal = phaseEnd - phaseBegin;

return retVal;

[ladyada]

hiya @Michele61 would you be interested in submitting this as a PR so we can get it included/fixed :)

[Michele61]

He ladyada, yes you can.

[ladyada]

great, we'll review when you make the PR

[Michele61]

Excuse me ladyada, but i don't know how the create a pull request, after that i have created a repository, where i written the code (https://github.com/Michele61/Adafruit-PWM-Servo-Driver-Library-fixed), what should i do ?

[Bolukan]

Use a fork

[Bolukan]

I took a look at your code: the OFF moment can be lower or higher than the ON moment and both numbers are 12 bits.

[ladyada]

hi check this tutorial
https://learn.adafruit.com/an-introduction-to-collaborating-with-version-control
https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github

[Bolukan]

I rethought this issue. Of course the function can calculate the number of ticks it is ON [0-4096]. But I think this will not suffice as a mirror of setPWM().
I have seen applications that set specifics start and end moments of each channel, p.e.

Channel	Start	End	Length
0	512	1535	1024
1	1536	2559	1024
2	2560	3583	1024
3	3584	511	1024

So I guess the function should return more information than just the length. Opinions?

[Michele61]

In the above example I calculate phaseBegin and phaseEnd, if the function has to return more information you can define them as a 'struct', in this way you obtain the same information of setPwm().

[Bolukan]

@Michele61, could you (and others) add use examples?
The function did not function for long time, so I guess it is not mainstream. That said, another possibility is to remove the function, till some need reappears. And if a specific use case exist, the function could be modelled like so. So not naming it getPWN but like getDutytime.

[photodude]

Use Case, I can't think of one here. I don't think you can get any useful information back, it's not like the pins can be used for reading digital or analog data, not sure what the purpose would be.

Click to expand my attempt at reading the code, parent libraries, and chip documentation to find a solution

I'm not sure If I'm following this right... but it seems getPWM(int num) is just wrapping the wire functions Wire.requestFrom(address, quantity, stop) and wire.read()
wire.requestFrom() returns "byte : the number of bytes returned from the slave device"
and wire.read() then reads the byte from the call to requestFrom()

digging deeper we are calling requestFrom((int)_i2caddr, PCA9685_LED0_ON_L + 4 * num, (int)4);

• our address _i2caddr (the i2C address set in the constructor... assume default 0x40)
• a byte quantity to get 0x06+4*num I assume num represents the needed offset for reading the registry section for the relevant pin
  • 0x06 +4 * 0 = 6 +4 *0 = 6 + 0 = 6
  • 0x06 +4 * 1 = 6 +4 *1 = 6 + 4 = 10
  • 0x06 +4 * 2 = 6 +4 *2 = 6 + 8 = 14
  • 0x06 +4 * 3 = 6 +4 *3 = 6 + 12 = 18
  • 0x06 +4 * 4 = 6 +4 *4 = 6 + 16 = 22
  • 0x06 +4 * 5 = 6 +4 *5 = 6 + 20 = 26
  • 0x06 +4 * 6 = 6 +4 *6 = 6 + 24 = 30
  • 0x06 +4 * 7 = 6 +4 *7 = 6 + 28 = 34
  • 0x06 +4 * 8 = 6 +4 *8 = 6 + 32 = 38
  • 0x06 +4 * 9 = 6 +4 *9 = 6 + 36 = 42
  • 0x06 +4 * 10 = 6 +4 *10 = 6 + 40 = 46
  • 0x06 +4 * 11 = 6 +4 *11 = 6 + 44 = 50
  • 0x06 +4 * 12 = 6 +4 *12 = 6 + 48 = 54
  • 0x06 +4 * 12 = 6 +4 *13 = 6 + 52 = 58
  • 0x06 +4 * 12 = 6 +4 *14 = 6 + 56 = 62
  • 0x06 +4 * 12 = 6 +4 *15 = 6 + 60 = 66
• stop 4 (this is not a bool???)

so we get between 6 and 66 bytes in the return and input an int for the stop value (I'm assuming that int 4 is ignored in the bool field).

Am I reading that right?

Seems like this function is not using wire.requestFrom() correctly and is why it's behavior is wrong.

So the question is how many bytes should be requested from the PCA9685 chip for the relevant information on each pin?
first glance (untested)

1. drop the int 4 as that's not a correct input
2. adjust the requested bytes (not sure what adjustment is needed)
3. read() the bytes
4. parse the bytes
5. return the parsed bytes

this is my start... needs to adjust the bytes see reference this is a brute force attempt at fixing (again untested)

uint8_t Adafruit_PWMServoDriver::getPWM(uint8_t num)
{
  _i2c->requestFrom((int)_i2caddr, 16);
  if(_i2c->available()<=16)
  {
    uint8_t X0 = _i2c->read(); // Reads the data from the register0
    uint8_t X1 = _i2c->read(); // Reads the data from the register1
    uint8_t X2 = _i2c->read(); // Reads the data from the register2
    uint8_t X3 = _i2c->read(); // Reads the data from the register3
    uint8_t X4 = _i2c->read(); // Reads the data from the register4
    uint8_t X5 = _i2c->read(); // Reads the data from the register5
    uint8_t X6 = _i2c->read(); // Reads the data from the register6
    uint8_t X7 = _i2c->read(); // Reads the data from the register7
    uint8_t X8 = _i2c->read(); // Reads the data from the register8
    uint8_t X9 = _i2c->read(); // Reads the data from the register9
    uint8_t X10 = _i2c->read(); // Reads the data from the register10
    uint8_t X11 = _i2c->read(); // Reads the data from the register11
    uint8_t X12 = _i2c->read(); // Reads the data from the register12
    uint8_t X13 = _i2c->read(); // Reads the data from the register13
    uint8_t X14 = _i2c->read(); // Reads the data from the register14
    uint8_t X15 = _i2c->read(); // Reads the data from the register15
  }
switch (num) {
  case 0:
    return X0;
   break;
  case 1:
    return X1;
   break;
  case 2:
    return X2;
   break;
  case 3:
    return X3;
   break;
  case 4:
    return X4;
   break;
  case 5:
    return X5;
   break;
  case 6:
    return X6;
   break;
  case 7:
    return X7;
   break;
  case 8:
    return X8;
   break;
  case 9:
    return X9;
   break;
  case 10:
    return X10;
   break;
  case 11:
    return X11;
   break;
  case 12:
    return X12;
   break;
  case 13:
    return X13;
   break;
  case 14:
    return X14;
   break;
  case 15:
    return X15;
   break;
}

So the question is how many bytes should be requested from the PCA9685 chip for the relevant information on each pin? Is there a better way to do this...

or even why are we not just using the read8() helper function in this library?

uint8_t Adafruit_PWMServoDriver::getPWM(uint8_t registerAddress)
{
   uint8_t read8(PCA9685_LED0_ON_L + 4 * registerAddress)
}

[pmaasz]

I can imagine some use cases. When writing software for a four legged robot you usually use invert kinematics to calculate the degrees/dutycicles with the given coordinates an actuator should move to. If for error handling or validation you could read out the pwm. Also the InMoov project relies on potentiometers for the arms to read out positions. I read a blog post by the creator that the power consumed by the robot was way too high to get it untethered. Reducing the amount of electronic parts would be a step in that direction. I know a poti is not a big amount of power consumption but it's a start.

I have to admit that I am a bit inexperienced in electrical engineering on higher levels so I am open to be schooled ;). I just like to tinker.

@Bolukan maybe the function should get split into three functions like getPWMStart(), getPWMEnd() and getPWMLength() with all three taking the pin number as input parameter?

[tbolin]

I agree that knowing the current pwm value of a pin might be use full. However, I can't think of a situation where it wouldn't be better to just store the value on the micro controller after sending it to the pwm board. Reading the values back over the i2c buss is so slow that you would have to be really memory starved to consider it an option, and even then there would probably be several better solutions available.

[BanksySan]

Seems to always return 0 now.

void loop() {
  // Drive each servo one at a time using setPWM()
  Serial.println(servonum);
  for (uint16_t pulselen = SERVOMIN; pulselen < SERVOMAX; pulselen++) {
    pwm.setPWM(servonum, 0, pulselen);
Serial.println("PWM:  " + String(pwm.getPWM(0)));
  }

  delay(500);
  for (uint16_t pulselen = SERVOMAX; pulselen > SERVOMIN; pulselen--) {
    pwm.setPWM(servonum, 0, pulselen);
 Serial.println("PWM:  " + String(pwm.getPWM(0))); }

  delay(500);

}

[caternuson]

For anyone having this issue, please update this library to latest release and try again. Note you may also need to install this library as it is now a dependency:
https://github.com/adafruit/Adafruit_BusIO

Closing. Can't recreated. Possibly fixed with BusIO update?

Test sketch:

#include <Adafruit_PWMServoDriver.h>

Adafruit_PWMServoDriver pwm = Adafruit_PWMServoDriver();

void setup() {
  Serial.begin(9600);
  while (!Serial);

  Serial.println("PCA9685 PWM set/get test.");

  pwm.begin();

  pwm.setPWM(0, 1234, 5678);

  Serial.print("ON  = "); Serial.println(pwm.getPWM(0, false));
  Serial.print("OFF = "); Serial.println(pwm.getPWM(0, true));
}

void loop() {
}

Serial Monitor output: