getPWM returns random numbers about adafruit-pwm-servo-driver-library HOT 19 CLOSED

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.

from adafruit-pwm-servo-driver-library.

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

from adafruit-pwm-servo-driver-library.

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.

from adafruit-pwm-servo-driver-library.

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.

from adafruit-pwm-servo-driver-library.

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;

from adafruit-pwm-servo-driver-library.

He ladyada, yes you can.

from adafruit-pwm-servo-driver-library.

great, we'll review when you make the PR

from adafruit-pwm-servo-driver-library.

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 ?

from adafruit-pwm-servo-driver-library.

Use a fork

from adafruit-pwm-servo-driver-library.

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.

from adafruit-pwm-servo-driver-library.

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

from adafruit-pwm-servo-driver-library.

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.

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

from adafruit-pwm-servo-driver-library.

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().

from adafruit-pwm-servo-driver-library.

@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.

from adafruit-pwm-servo-driver-library.

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.

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;
}

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

from adafruit-pwm-servo-driver-library.

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?

from adafruit-pwm-servo-driver-library.

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.

from adafruit-pwm-servo-driver-library.

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);

}

from adafruit-pwm-servo-driver-library.

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:

from adafruit-pwm-servo-driver-library.