Arduino_APDS9960

[SpenceKonde]

Discussed in https://github.com/SpenceKonde/megaTinyCore/discussions/707

^{Originally posted by **atghrr** May 16, 2022} Hello! I'm trying to use default Arduino library to work with apds9960, but a compilation error occurs. Apds9960 uses I2C interface. The chip is Attiny3227. Previously, when i used atmega168(328) it was workable. Please help... There is a log of Arduino IDE: C:\Users\1\Documents\Arduino\libraries\Arduino_APDS9960\src\Arduino_APDS9960.cpp: In member function 'size_t APDS9960::readBlock(uint8_t, uint8_t*, unsigned int)': C:\Users\1\Documents\Arduino\libraries\Arduino_APDS9960\src\Arduino_APDS9960.cpp:261:41: error: call of overloaded 'requestFrom(int, unsigned int&)' is ambiguous _wire.requestFrom(APDS9960_ADDR, len); ^ In file included from C:\Users\1\Documents\Arduino\libraries\Arduino_APDS9960\src/Arduino_APDS9960.h:24:0, from C:\Users\1\Documents\Arduino\libraries\Arduino_APDS9960\src\Arduino_APDS9960.cpp:20: C:\Users\1\Documents\ArduinoData\packages\megaTinyCore\hardware\megaavr\2.5.11\libraries\Wire\src/Wire.h:106:13: note: candidate: uint8_t TwoWire::requestFrom(uint8_t, uint8_t) uint8_t requestFrom(uint8_t address, uint8_t quantity); ^~~~~~~~~~~ C:\Users\1\Documents\ArduinoData\packages\megaTinyCore\hardware\megaavr\2.5.11\libraries\Wire\src/Wire.h:108:13: note: candidate: uint8_t TwoWire::requestFrom(uint8_t, size_t) uint8_t requestFrom(uint8_t address, size_t quantity); ^~~~~~~~~~~ C:\Users\1\Documents\ArduinoData\packages\megaTinyCore\hardware\megaavr\2.5.11\libraries\Wire\src/Wire.h:110:13: note: candidate: uint8_t TwoWire::requestFrom(int, int) uint8_t requestFrom(int address, int quantity); ^~~~~~~~~~~ exit status 1

[SpenceKonde]

This is probably the most infuriating things about how overloading works in C++

We currently have

uint8_t requestFrom(uint8_t address, uint8_t quantity, uint8_t sendStop);
uint8_t requestFrom(uint8_t address, uint8_t quantity);
uint8_t requestFrom(uint8_t address, size_t  quantity, bool    sendStop);
uint8_t requestFrom(uint8_t address, size_t  quantity);
uint8_t requestFrom(int     address, int     quantity, int     sendStop);
uint8_t requestFrom(int     address, int     quantity);

I guess that isn't good enough.

Maybe I need to gothrough every conceivable permutation of the arguments.... With each one as either uint8_t or int for the first argument, uint8_t, size_t, and int for the second, and bool, uint8_t, and int for the third. And I guess this implies that we need to also support references being passed for each argument too...

That would be all well and good if we were doing anything different depending on the size of the argument,. But we don't; all these functions are identical except the one with all uint8_t's (I2C addresses are 7 bits plus the r/w bit, so that's fine in 8, the I2C buffers are not larger than 32b on tinyAVR parts (and never exceed, iirc, 130 bytes on an AVR (I did that on the Dx-series specifically to allow you do do a whole page write to an AT24-series EEPROM chip with them) - it just truncates tha arguments to uint8_t's and calls that version...

There has got to be a more efficient way than this!

[atghrr]

With SparkFun_APDS9960 Library the situation is similar:

C:\Users\1\Documents\Arduino\libraries\SparkFun_APDS9960_RGB_and_Gesture_Sensor\src\SparkFun_APDS9960.cpp: In member function 'int SparkFun_APDS9960::wireReadDataBlock(uint8_t, uint8_t*, unsigned int)': C:\Users\1\Documents\Arduino\libraries\SparkFun_APDS9960_RGB_and_Gesture_Sensor\src\SparkFun_APDS9960.cpp:2204:44: error: call of overloaded 'requestFrom(int, unsigned int&)' is ambiguous Wire.requestFrom(APDS9960_I2C_ADDR, len); ^ In file included from C:\Users\1\Documents\Arduino\libraries\SparkFun_APDS9960_RGB_and_Gesture_Sensor\src\SparkFun_APDS9960.cpp:20:0: C:\Users\1\Documents\ArduinoData\packages\megaTinyCore\hardware\megaavr\2.5.11\libraries\Wire\src/Wire.h:106:13: note: candidate: uint8_t TwoWire::requestFrom(uint8_t, uint8_t) uint8_t requestFrom(uint8_t address, uint8_t quantity); ^~~ C:\Users\1\Documents\ArduinoData\packages\megaTinyCore\hardware\megaavr\2.5.11\libraries\Wire\src/Wire.h:108:13: note: candidate: uint8_t TwoWire::requestFrom(uint8_t, size_t) uint8_t requestFrom(uint8_t address, size_t quantity); ^~~ C:\Users\1\Documents\ArduinoData\packages\megaTinyCore\hardware\megaavr\2.5.11\libraries\Wire\src/Wire.h:110:13: note: candidate: uint8_t TwoWire::requestFrom(int, int) uint8_t requestFrom(int address, int quantity); ^~~ exit status 1

[MX682X]

Kinda bored right now so I looked up how it might be possible to solve it. Can't test it right now either, but how about something like this:

uint8_t requestFrom(auto address, auto quantity, auto sendStop = true) {  //'auto' automatically assigns the right type on compiletime
static_assert(     //creates compiletime error if false
  std::is_same_v<decltype(address), int8_t> ||    //checks if both are of the same type. we don't want pointers
  std::is_same_v<decltype(address), uint8_t> ||
  std::is_same_v<decltype(address), int16_t> ||
  std::is_same_v<decltype(address), uint16_t> ||
  std::is_same_v<decltype(address), int> 
  , "address variable has incopatible type");  //adds message to error
//repeat for quantity and sendStop

return request(uint8_t) address, (uint8_t) quantity, (bool) sendStop); 
}

[MX682X]

Nevermind, the stuff does not work.

Furthermoe, I tried to recreate the error by copying the function call, but it compiled with no problems for me. So I can't really debug it. What confuses me most is that I can't tell where unsigned int&comes from. I'll try to use the library tomorrow.

On a side note about overloading: It seems like one function, uint8_t requestFrom(int16_t address, int16_t quantity, int16_t sendStop = 0x01); should be able to cover most cases, since if there is no exact match, the compiler exapnds the 8-bit types to next bigger types automatically.

[SpenceKonde]

Looking at this now, I'm confused as to why it wouldn't work to just have the one version.... requestFrom(uint8_t address, uint8_t quantity, uint8_t sendStop = 1)? That way we don't pass around larger types than are necessary...

[MX682X]

I'd suggested to use uint16_t because size_t is an (u)int16_t and arduino ints, which are often used in examples and tutorials, are also 2 bytes wide. Besides, the compiler prefers to have int anyway, as you can see in the posts above (call of overloaded 'requestFrom(int, unsigned int&)' is ambiguous ). I'm just afraid there will be warnings like "can't convert types" or so, because the function expects a thinner variable.

[SpenceKonde]

Well, that's the question, can we make it generate warnings with 8-bit values? If not, it s strictly better to have the datatype tuncacted ASAP because every time we pass an extra byte in the type of a variable here, IIRC, this is 2 or 4 bytes. so the difference is 6 or 12 bytes!, in other words, up to 0.6% of the total flash. Wasting flash like that for things that are not optional and documented as less efficient options is not acceptable unless it would be in conflict with a higher priority rule, like "The core will never generate warnings, any warnings you see are4 the result of your code or a third party library" (this also happens to be why I'm comfortable rigging up the platfiorm.txt so that there's no ACTUAL way to turn off warnings. Setting them to none doesn't do anything :-P

Also, remember, there are users on 2k flash parts, They are very squeaky wheels, any increase in binary size leads to angry issues, public denunciations and crowds of angry users outside my apartment with pitchforks and torches.

[MX682X]

Ok. so finally got to test it with the libraries themselves. I've tested it with Adafruit_APDS9960.h (gesture sensor example) and SparkFun_APDS9960.h (Color Sensor example).

Adafruit compiled with the overload mess at 5610 bytes on 3227, sparkfun did not

using my solution above (int16_t) with a single function Adafruit compiled with 5602 bytes and sparkfun with 4920 bytes

using uint8_t or int8_t increased the flash size to 5610 bytes on adafruit again. sf stayed the same with int32_t , the size bloated up to 5642 with adafruit. sf stayed the same

and I have no idea how this happend, considering they (adafruit) already casting it to uint8_t: size_t recv = _wire->requestFrom((uint8_t)_addr, (uint8_t)len, (uint8_t)stop); looking through the .lst files didn't really help me understand it either. Well, gonna blame the implementation of the library then....

But I found something else that should help reduce the codesize - by avoiding storing and loading the address we can save 8 bytes, in the worst case it takes only one cpu register. (Master read only)

 uint8_t TwoWire::requestFrom(int16_t  address,  int16_t  quantity,  int16_t sendStop) {
  if (quantity > BUFFER_LENGTH) {
    quantity = BUFFER_LENGTH;
  }
  //vars._clientAddress = (uint8_t)address << 1;
  return TWI_MasterRead(&vars, (uint8_t)address, (uint8_t)quantity, (uint8_t)sendStop);
}

In order to see how the compiler treats the arguments or rather when the casting occured, I've used Wire.requestFrom(addr++, count++, stop); stop ^= 1; (all uin16_t) with arguments of the type int16_t and uint8_t, and the latter ended up being a bit shorter and din't result in any warnings. So I guess we can reduce everything to

uint8_t requestFrom(uint8_t address, uint8_t quantity, uint8_t sendStop = 0x01);

On a side note: have you considered making a "Stream Light" Class? Stream always initializes an uint32_t variable for timeout, effectively wasting 4 bytes with each Wire Object. Not to mention the unused virtual functions.... Edit: there are actually 2 unsigned long variables for timeout

[technoblogy]

I can't quite understand from this thread whether the SD library in:

megaTinyCore/hardware/megaavr/2.5.11/libraries

should work with chips such as the ATtiny1614. I've tried it and I can't get it to work, but I may be doing something wrong.

Thanks!

[SpenceKonde]

@technoblogy Yes it absolutely should work. Are you having problems? Please creeate a separate issue, this one relates to I2C, while SD cards use SPI.

[technoblogy]

I thought I'd posted it in

We need an SD card library - anyone ported this one yet #226

I don't know how it ended up here - sorry.

[SpenceKonde]

Please make sure there's an open issue if there is an open problem. Don't comment on an old issue that's closed and leave it closed if you have evidence the probem is not resolved. I probably won't notice it. Open issues are my list of action items!

Considering how old that problem is (we have belived that SD worked for years!) I think this should be a new issue, containing code that reproduces the problem and a description of the wrong behavior. I don't yet have the information I'd need in order to even contemplate the issue, since no code, no description of failure mode.

[SpenceKonde]

@MX682X My concern with light stream class is that a million things subclass stream, and I'm concerned with the possibility if we start using our modified light streams and there being compatibility issues. I would love to make stream more efficient though, the bloat on stream and printable is enough to make one ill.

[atghrr]

I have hardware and can test the solution. How should i modify core at my local machine? Or what should i do in order to test?

[MX682X]

I have hardware and can test the solution. How should i modify core at my local machine? Or what should i do in order to test?

What code do you want to test? Just the fix on requestFrom(), or do you want some immprovemnts on the Stream library?

[atghrr]

First, let's check the work with the fix. And then I can check other options.

[atghrr]

I did the following: 1) leaving only one option at Wire.h, with correction from uint8_t to uint16_t

uint16_t requestFrom(uint16_t address, uint16_t quantity, uint16_t sendStop);
uint16_t requestFrom(uint16_t address, uint16_t quantity);

2) leaving only one option at Wire.cpp, with correction from uint8_t to uint16_t

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

Did I do the right thing?

[atghrr]

I think it was wrong, because APDS sensor did not work. Tried another: 1) leaving only one option at Wire.h, without modifications of data type

    uint8_t requestFrom(uint8_t address, uint8_t quantity, uint8_t sendStop);
    uint8_t requestFrom(uint8_t address, uint8_t quantity);

2) leaving only one option at Wire.cpp, without modifications of data type

uint8_t TwoWire::requestFrom(uint8_t  address,  uint8_t  quantity)                   {
         return requestFrom((uint8_t) address, (uint8_t) quantity, (uint8_t) 1);
}
uint8_t TwoWire::requestFrom(uint8_t  address,  uint8_t  quantity,  uint8_t sendStop) {
  if (quantity > BUFFER_LENGTH) {
    quantity = BUFFER_LENGTH;
  }
  vars._clientAddress = address << 1;
  return TWI_MasterRead(&vars, quantity, sendStop);
}

And it works, sensor succesfully transmits gesture information. It was tested with default Arduino library,

[MX682X]

yeah, I didn't get to answering you before. uint8_t everywhere is the intended way. Good that it works with the HW too

[SpenceKonde]

Wonderful. I will roll out this change with the next version of the core which will be released before the end of the week,

[SpenceKonde]

Okay several weeks later I get this change committed