TWI name classification not perfect. (Information Request)

[mikrocoder]

Hallo,

I'm dealing with TWI right now. I noticed that your naming scheme is not 100% the same. The TWI buffer length 'BUFFER_LENGTH' should have the name TWI_BUFFER_LENGTH. Otherwise it will be too easily overwritten by own defines. This is the big disadvantage of C and defines. They are visible everywhere without access protection.
BUFFER_LENGTH is actually a common name for something like that for itself. So it can collide very easily.

After renaming this to TWI_BUFFER_LENGTH please,
I would remove the check in twi.h.
#ifndef TWI_BUFFER_LENGTH
I would define TWI_BUFFER_LENGTH hard depending on RAMSIZE.
Then a programmer has when compiling the chance of an error message when trying to create multiple identically named defines. Otherwise the programmer gets no warning and the value is overwritten or skipped if necessary.

Files: twi,h, twi.c and Wire.cpp
Would you please change that?

Is certainly in the MegaCoreX the same problem. I think.

[MX682X]

This is kinda on my to do list. I've planning to get rid of the c/c++ seperation and planning to put everything in Wire.cpp/Wire.h. The change would include changing the define name to TWI_BUFFER_LENGTH, with the possibility to chnage it through own defines.... Maybe even offering a different API using own buffers/pointers when TWI_BUFFER_LENGTH == 0

Edit: Let's say ETA: about 3 weeks

[mikrocoder]

That's nice to know. Thank you.

[SpenceKonde]

Yes, the behavior of defines and includes is, in sum total, wacky, and the name of that define is not a good one. One thing I worry about whenever I'm asked to rename something is whether there is any code in the wild that makes use of the current name. It matters a great deal what the context is: If everyone else calls it TWI_BUFFER_LENGTH, it's unarguably a bug. But if the stock core and most third party ones call it BUFFER_LENGTH, then we have to worry about the user who - there's an awkward issue though, and that is that I think BUFFER_LENGTH is defacto standard for Wire implementations; if just my Wire uses that, and everyone else uses TWI_BUFFER_LENGTH, that's a bug and I should fix it. But if everyone calls it BUFFER_LENGTH, then we can't get rid of the old name because extant code likely relies upon it.

We do set the BUFFER_LENGTH based on RAMSIZE.
We just happen to not choose from very many bins - only when the RAM won't be noticed do we expand it, or when the part would not be plausibly viable with the 32-byte buffer.

1. If you don't have 32 byte buffers, tons of shit doesn't work. I2C device libraries generally assume a 32b buffer. Nobody checks to see that all the bytes they tried to send were sent. So we do 32 even when it hurts, because the alternative is that so much stuff is broken that it defeats the point of a compatible library). The only time we don't is when the part has so little ram that it is not a plausibly useful Wire device with 32 bytes allocated to the buffer (remember there are typically 2 buffers, that's half the ram on a attiny2[01][24], and it's highly unlikely that a library that relies ion the write buffer size like many LCD libraries would fit in 2k of flash, and as soon as you have 4k+ you're back to the normal 32b - The test is RAMSIZE < 256 to get the half-size buffer.
2. If you allocate more than 32 bytes... say we allocated 64? Who would use it? what code? Not a normal library, they all assume 32b, and that's a safe assumption and there's little in it for them to try to squeeze a tiny bit more performance out on mid-range parts by figuring out the parameters of the Wire library such that they know the size of it's buffer, and can thus write exactly that much at a time; I would expect to see at most one library make use iof uit not counting ones I might write). And of course there's the ever-present question of where would you draw the boundaries, and what about someone asking for just a little more buffer? More likely, someone developing with one of these parts will think they're developing with vanilla wire, and then find out that they were unwittingly depending on the buffer size being larger than normal, especially if we had more levels of
3. I do, however, on parts with 4k or more ram, bump the size up by 2 notches plus two - from 32 to 130. 128 is a natural value, but the penalty of not having it as a power of two is small. It's also used in a very plausible use case - that of an external I2C EEPROM (inexpensive, one of the most simple things ever to interface with) (see below) - the top end sizes (the "good ones") have 128b pages, and take 2 bytes of address to access the 64kb on the 24LC512 (512 is kbit, a tradition in memory size naming); larger sizes start taking the low 3 bits of the device address, and no I2C eeproms that let you have more than 4 mbit on one I2C bus witout actively driving the address lines like a chip select.

I very often use external EEPROMs, so obviously that bias had an impact. But the modern AVRs also represent a stark departure from the precedents of classic in a way many don't immediately notice: The ratio of the three memory types has not changed drastically for flash+RAM, nor did it vary greatly on classic. Some typical classic AVR ratios: (Flash first, eeprom last)

16 :  1 : 1 (t87)
32 :  1 : 1 (t167)
32 : 2 : 1 (common)
64 : 2 : 1 (m2560)
32 : 4 : 1 (1284p)

See, you never had EEPROM smaller than 1/4th of ram
On modern AVR:

64 : 8 or 6 : 1 or 2 - tiny2
64 : 4 : 1 - tiny0 or tiny41x/81x
128: 8 or 16 : 2  or 1 - tiny 161x/321x (obviously flash is less than 128k on these parts, but this is a ratio so what's the problem?)
256:32:1 - 128k DA/DB, 64k DD
128:16:1 - 64k DA/DB, 32k DD
64 : 8 :1 - 32k DA/DB, 16k DD
32 : 4 :1 - 16k EA
64 : 8 :1 - 32k EA
128 : 12 : 1 - 64k EA

Highest eeprom relative to flash or ram is the lowest eeprom ratio seen on a classic.

I think you see the point here. While the ram ratio has gotten less confining (assuming you compare on basis of flash, but whatever basis same conclusion is drawn), the EEPROM has gotten a lot smaller relative to other memories; the classic AVR with the least EEPROM for it's flash (among reasonably large parts) is the m2560/1, with 4k EEPROM. Only the 16k AVR EA series have a lower flash:eeprom ratio than the highest classic ratio. My point is that these parts are comparatively thin on EEPROM. Well no big deal just use the flash? But it seems that some sort of mass psychogenic illness has taken hold within Microchip over the past three or four years, causing all of the engineers to forget how to make flash memory. They've redesigned nvmctrl twice, once for Dx, once again (starting from tinyavr version) for Ex. The Dx, obviously, looked like everything was going swimmingly, until someone running the part near the top of it's temp range observed that the flash crapped out sooner than it was supposed to; at that point Microchip amended their Silicon Errata and Datasheet Clarification to cover this; The table listing the flash parameters is included, with the flash endurance line "clarified". Now one of the zeros is so clear, you can see right through it!

So nobody was surprised to hear that the EA would have a new nvmctrl - though I'm sure many of us were sad when we learned that it wouldn't have word write.

Then the EA's errata sheet hit.

Rev. B1
Device 2.2.1. NVM Programming Does Not Work Below 2.7V 
2.2.2. Reduced Flash Endurance for VDD Below BODLEVEL3 
2.2.3. Write Operation Lost if Consecutive Writes to Specific Address Spaces 
CRCSCAN 2.3.1. Running CRC Scan on Part of The Flash is Non-Functional 
NVMCTRL 2.4.1. Flash Multi Page Erase Non-Functional from UPDI
2.4.2. Flash-Self Programming Failing When Flash Read During Programming

You thought losing 90% of your flash endurance is bad huh? At least that's the only important thing broken in the nvm department on Dx). How about self programming not working reliably at all below 2.7v, and losing 90% of endurance below 4.5V? Oh and you can't multipage erase from UPDI. And you know how we have rww where the nrww flash can be used while the rww section is being written? So, these parts have a page buffer so that will make sense. Unfortunately, though it doesn't stop execution anymore - apparently if "data is read" during the write or erase, it can fail to write the flash!

I'm going to ask for clarification about what that means. I know LPM would do it. I figure accessing via memory mapped flash would do it, and you've gotta not do that,. They say instruction fetch is okay, (thank god). One thing I just don't know is the extent to which real world bootloader code will trigger it. If it's just non-instruction-fetch reads of the nrww section, like, just don;t do that when there could be a write going, it's not like you have much to read from the flash during a write until you're done writing, at which time you start reading to verify.

But BACK ON TOPIC, the fact that these parts are so poorly eeprom'ed and that there is an extra large helping of errata on the menu w/rt self programming flash, is part of how I justify ever going above 32b for the buffer at all.

Unclear about what you are referring to about the files?

That's the way Wire has always been organized, everywhere. I belive (just from how the code looked before it was rewritten) that one person likely affiliated with Arduino did Wire.*, but a different person wrote the twi.h/twi.c - I suspect a microchip/atmel person wrote it as a reference implementation. In any event, post rewrite, Wire retained the same structure; the pattern of a C++ wrapper around C functions is widespread, in and out of embedded systems. So yeah, not sure what your issue is?

[mikrocoder]

Hi,

What are you writing?
MX682X has already understood me correctly. If MX682X has rebuilt the lib and everything has its access protection, he can name the buffer as he wants. That interests then nobody more. The name remains Lib internally.
For people who want to work with the current buffer size MX682X can provide a getter method.
32 bytes is also not the default if you reduce it to 16. See twi.h.

So yeah, not sure what your issue is?

Please think about the impact that defines have on the whole program. How the preprocessor works. For example I can't use in my enums names that you use in your core as defines.

Do you know where in the core you use which define names?
BUFFER_LENGTH is a name for everything. It can be used for TWI, for USART, for SPI and so on.
In the twi.h this is stupidly queried with #ifndef BUFFER_LENGTH. If someone somewhere creates a #define BUFFER_LENGTH, before including the Wire.h, your BUFFER_LENGTH will be ignored mercilessly and has the wrong value.

Since I know you use defines for everything, which I don't like, the least you could do is to name the buffers according to the hardware unit. And not to work with #ifndef.
Ex. instead of:

#ifndef BUFFER_LENGTH
  #if (RAMSIZE < 256)
    #define BUFFER_LENGTH 16
  #elif (RAMSIZE < 4096)
    #define BUFFER_LENGTH 32
  #else
    #define BUFFER_LENGTH 130
  #endif
#endif

better like this:

  #if (RAMSIZE < 256)
    #define BUFFER_LENGTH 16
  #elif (RAMSIZE < 4096)
    #define BUFFER_LENGTH 32
  #else
    #define BUFFER_LENGTH 130
  #endif

If you or someone else accidentally tries to define the name BUFFER_LENGTH again, the compiler will at least report a warning. Otherwise you have no chance as a programmer.

If you take into account every mistake made back then, it might not compile for someone's code anymore, then what are you writing readme for the core updates for? If you always take into account old mistakes you will eventually come to a dead end.

I am sure MX682X will get it right.

[MX682X]

Maybe I should specify:
USART is using #if !defined(SERIAL_TX_BUFFER_SIZE) (similar for RX) and uses RAM size based buffers.

I was planning to rename the TWI define to TWI_BUFFER_SIZE to have a similar naming scheme.
I don't think there are many people changing their platform.txt to another Buffer size, afterall, everything is build around this size.
Also, if people decide to upgrade the DxCore, they need to change the platform.txt anyway as it is overwritten otherwise.

Maybe I could add a

#if defined(BUFFER_SIZE) && BUFFER_SIZE != 32 
#warning "This define was renamed. Please use TWI_BUFFER_SIZE instead" 
#endif 

besides writing it down in the changelog. Worst case scenario? people will have to add another -D option to the command line to have a platform.txt compatible with the old and new version of Wire.

I just want to avoid having a generic name for a specific library.

[mikrocoder]

Hi,

that all sounds reasonable. I trust you completely on that.
By the way, for deviations of the platform.txt I use a platform.local.txt. Then you don't have to change anything in the original platform.txt.

Maybe you can include a getter method that returns the buffer size value.
Example
uint8_t getBufferSize (void) const { return TWI_BUFFER_LENGTH; }
or better
constexpr uint8_t getBufferSize (void) { return TWI_BUFFER_LENGTH; }
Then TWI_BUFFER_LENGTH can stay in the class internally and externally you get the value e.g. with twi.getBufferSize() or so.
I leave that up to you.
Practical example. Then you can read and write larger data for example with a FRAM or EEprom. You divide it into pieces that correspond to the buffer size. You have the control for reading and writing at any time. Then you don't have to adjust the buffer size manually for larger data structures. :-)

By the way. If you can, do not define variables with #define, but use constexpr with datatype. This is also used 100% at compiletime. This would solve the unintentional overwriting of existing variables.

[MX682X]

It is an intentional design to overwrite the Buffer Sizes by defines, as you can pass #defines on the command line with -D

[mikrocoder]

Why would you want to do that? I mean overwriting. Because defines have other side effects.

[SpenceKonde]

What MX682X said.

I agree that that is a bad name, and as a result, unintentional collisions are more possible than they otherwise would be - but your proposal isn't an improvement - what your proposal amounts to is "You want to change the buffer size? Tough, modify the library" (which is buried 5 layers of folders down in a hidden folder on most peoples system).

The thing is, a BUFFER_SIZE define or after some set of changes TWI_BUFFER_SIZE can only be changed if it comes from platform.txt/boards.txt/whatever to get the -D for it in. If a different buffer size is specified in a #define before #including wire, you get bizzaro broken results due to the way includes work. While compiling the sketch, it will have used the user-supplied value within the compilation unit associated with the .ino.cpp, while Wire.cpp would include Wire.h. It doesn't have a user-supplied value visible to it, so it uses the default, and then whether the linker notices and barfs or links a broken binary, the result is no good, because the code has different definitions of TwoWire.

This in general is a problem where you have a library that has a configurable but compiletime constant parameter that you would like to allow people to adjust, but it requires conditional compilation in order to implement efficiently. That means that there is no way to get that parameter to it outside of it's modifying it's header file or a global -D (preferable to modified library, as there's only one file containing all the stuff you fucked with, and you can avoid a core update unfucking it by using platform local). There's no way to propagate an argument from the application to a library if it needs to act as a macro. All you can do is blow everything up trying, by causing the class to have different definitions depending on the file that's including the library,

[mikrocoder]

Hi,

do we always misunderstand each other? I do not know why. Whether it is because of the translation. I don't know.

I don't want to change any buffer size. If I change it, I do it in the twi.h. I'm programming a lib for myself which is based on the Wire.h. While programming I noticed the ambiguous name BUFFER_LENGTH, which I think should be changed. Furthermore it is "global" because of define. That's a bit of a ticking time bomb. More I did not want to communicate. Because MX688X wants to change the lib anyway, wait and see what all changes. ;-)

[mikrocoder]

Hello again,

I have discovered a problem with the buffer size. With write() always 2 bytes too less are written or transferred as the buffer size is. To see this more clearly, I have set the BUFFER_LENGTH to 10 (twi.h) and transfer 10 bytes into my FRAM.
On the Logic Analyzer I see that only 8 bytes are transferred.
Reading with read() works. So 10 bytes are read.
Can it be that with write and the ring buffer something does not work?

Stream &cout {Serial2};
#include <Streaming.h>
#include <Wire.h>

TwoWire &wire0 {Wire};

constexpr bool DEBUG {false};
constexpr uint8_t  i2cAddr {0x57};
constexpr uint16_t FRAM_INITIAL_CELL_ADDR {100};
const uint8_t datenWrite [] {0,1,2,3,4,5,6,7,8,9};
uint8_t datenRead [sizeof(datenWrite)] {};
    
void write(const uint8_t i2cAddr, TwoWire &line, const uint32_t cellAddr, auto &data)
{    
  uint8_t error {0};  
  line.beginTransmission(i2cAddr); 
  line.write(static_cast<uint8_t>(cellAddr >> 8) );  // MSB
  line.write(static_cast<uint8_t>(cellAddr & 0xFF) );  // LSB
  for (auto &d : data) {
    line.write(d); 
    if (DEBUG) {cout << d << "  ";}
  }
  error = line.endTransmission();   
  cout << F("\nerror ") << error << endl;
}

void read(const uint8_t i2cAddr, TwoWire &line, const uint32_t cellAddr, auto &data)
{    
  uint8_t error {0};  
  line.beginTransmission(i2cAddr); 
  line.write(static_cast<uint8_t>(cellAddr >> 8) );  // MSB
  line.write(static_cast<uint8_t>(cellAddr & 0xFF) );  // LSB
  error = line.endTransmission();
  cout << F("error ") << error << endl;
  line.requestFrom(i2cAddr, sizeof(data)); 
  for (auto &d : data) {
    d = line.read(); 
    if (DEBUG) {cout << d << "  ";}
  }
  if (DEBUG) {cout << F("\nerror ") << error << endl;}
}

void setup()
{
  Serial2.swap(1);    // PF4 TXD2 / PF5 RXD2
  Serial2.begin(9600); 
  Wire.begin();
  cout.println("\nuC Reset ####\n");
  write(i2cAddr, wire0, FRAM_INITIAL_CELL_ADDR, datenWrite);
  read(i2cAddr,wire0, FRAM_INITIAL_CELL_ADDR, datenRead);
  for (auto &d : datenRead) {
    cout << d << "  ";
  }
  cout << endl;
}

void loop (void)
{  }

[MX682X]

You are writing 2 bytes of address and 10 bytes of data, making 12 bytes. As the buffer has only 10 bytes, the last two writes are discarded. Wire.Write will return 0 if the buffer is full

[mikrocoder]

If you look at it that way, yes. But should address bytes really belong to the user data? Is that the same with the original Arduino Wire.h? I'll have a look at it. I was always of the opinion that the buffer size is the user size. Because when reading also the device address and the address bytes are written before reading.

[MX682X]

I'm not talking about the I2C address, but the FRAM Address

line.write(static_cast<uint8_t>(cellAddr >> 8) );  //  1st write
line.write(static_cast<uint8_t>(cellAddr & 0xFF) );  // 2nd write
for (auto &d : data) {
    line.write(d); // write 3-12, with 11 and 12 failing
    if (DEBUG) {cout << d << "  ";}
}

In other words, you need a buffer that is bigger by two compared to your maximal data size.

[mikrocoder]

Hello, you're right. when writing, everything is written one after the other on the bus. When reading, the 2 address bytes from the initial write are already out of the buffer before it really starts reading. Thanks for the correction. ;-) I help myself now with

constexpr uint8_t TWI_BUFFER_SIZE {BUFFER_LENGTH-2};

One more additional question. In general, do TWI/I2C FRAMs always have only a maximum of 2 address bytes for the memory cell? Or could there be theoretically also some with 3 address bytes for the memory cells?