other formula for TWI BAUD calculation #243
Replies: 41 comments 25 replies
-
|
We have reimplemented the master SCL clock formula at least 4 times now.... maybe more... The way the clock generation is implemented in the hardware makes for very awkward configuration :-( I know the implementation Arduino used didn't work right, for a while there was an implementation that would result in an integer overflow in some cases, then I "fixed" it incorrectly, then fixed it incorrectly again, and that's when @MX682X stepped in and implemented something that worked, at least a lot better than it did before. (and nextthing he knew, he was completely reimplementing Wire - that's where the new wire implementation that uses less flash and supports dual mode came from) Are you saying that in fact there is still a problem with how we calculate it? It doesn't matter, as far as I am concerned, if the results are wrong when the clock frequency is 3.333 MHz - that is not a supported clock speed, and lots of things will break if you're running at that speed. Neither the core nore any core libraries are expected to work correctly at any speed that isn't on the list below. If they do, so much the better, but it's not considered a bug if they don't. The supported clock speeds are 1 MHz, 4 MHz, 8 MHz, 10 MHz, 12 MHz, 16 MHz, 20 MHz, 24 MHz, 25 MHz, 27 MHz, 28 MHz, 30 MHz, 32 MHz, 36 MHz, 40 MHz and 48 MHz (I think 3 MHz, 5 MHz, 6 MHz and 44 MHz, would work if specified from boards.txt, and maybe even 7 and 14 - but they are most certainly not "supported speeds") 27 MHz is in because it's used in some RF application and those folks want to use the same clock for the radio and the mcu, and it turned out to be easy to make the math work out - the hardest speed to do the micros math on was actually 24 MHz. And 28 is supported because it's an internal oscillator speed on the Dx-series (undocumented - CLKCTRL.OSCHFCTRLA FRQSEL bitfield according to datasheet tops out at 0x09, 24 MHz. The first thing I did when I read that during development of DxCore was see what happened when I tried 0x0A, 0x0B and so on. 0x0A and 0x0B give 28 and 32 MHz clock, ones above that repeat the last 4). 40 is supported because that's the highest speed that most parts will reach from an external crystal at room temp, and 48 is the highest speed reasonably obtainable with external clock. 25 MHz because apparently lots of people have piles ot 25 MHz crystals around and wanted to be able to use them (multiple people asked for it, lol) And 30 MHz is because it's a megaTinyCore speed - it's the fastest speed that most 1-series parts can be tuned to using the internal oscillator. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, I think there is a misunderstanding here, maybe due to my translation. I have not tried 3.3MHz, only mentioned. I understood your description as using the formula from the Arduino Lib. Then I would have read that wrong. I have programmed with 16Mhz crystal and Wire.setClock etc. an I2C scanner incl. frequency scan. The tests in 100kHz steps all addresses between 100kHz and 800kHz. Whereby with 16MHz anyway only up to 600kHz are real possible. My results which I had measured with it: Of course the clock depends on many. Only if I want a certain clock, then it should already be generated. If it is then practically not possible, due to too large pullups, bad cable etc., then one must pursue the cause. With which CPU clock did you check and measure your code? Maybe my formula for higher CPU clocks does not fit. Another question. Does the controller work stable with extremely higher clocks than 24MHz? Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
|
The thing is, the hardware does not provide a way for you get a "100kHz" clock. It is impossible to calculate the baud register value that will give 100kHz without knowing the rise time, which depends on the strength of the pullups and the capacitance of the bus. The value the register is set to is used by the hardware along with measuring when the pin went high to calculate when to start the next clock cycle.. A setting that gives 100kHz on a bus with one set of parts on it, may give results slightly faster or slower on a different bus with different lengths of wire and different number of parts; The library tries to guess a baud value that will be close to the requested one, but we will never be able to guarantee that you get the closest match possible. While it's not entirely unreasonable to have an offset argument to the setClock, I'm not sure how worthwhile it would be. We may very well not be picking the best offsets. Certainly we seem to come out quite a bit slow in @mikrocoder 's tests But maybe the offsets are fine, and his bus is slower than is typical. We have no idea what his I2C configuration is like physically. If the internal pullups are being used, or it there are a large number of I2C devices on the bus, or the wires are long, this might not be an indication of a problem with the library at all. Maybe those offset values are just fine, for typical conditions, and the lower speed is just a reflection the electrical properties of the bus..... Adding a feedback channel to measure the actual SCL clock would be the only way to make it always match, but the overhead of such a solution would make it impractical. |
Beta Was this translation helpful? Give feedback.
-
|
Ok, I think I can see the problem. It has probably something to do with the Offsets that are used in TWI_MasterCalcBaud() in twi.c . Can you try to remove them and measure the frequencies again, @mikrocoder . @SpenceKonde if the offsets are in fact creating a problem, maybe it would be useful to add an extra argument in the SetClock register for manual offsetting the baud register. Basically remove all offsets in the calculation and instead have a |
Beta Was this translation helpful? Give feedback.
-
|
@MX682X Can you confirm that the minimal change that would simply eliminate offsets so we can see the scale of the effect thy are having would be to change the final function in twi.c to: |
Beta Was this translation helpful? Give feedback.
-
|
Hello, I don't want to grumble, but I described and wrote everything in the first post. My formula and also in the measurement data that I have 3.3k pullups. This is an Board 17x11cm with a AVR128DB64. All I2C devices on the board work with up to 600kHz which is possible with 16MHz CPU clock. And yes 100kHz are possible. Why should this not be possible? +/- <1kHz. Furthermore, there is a problem of understanding. If you specify a SET frequency with which the baud value is to be calculated, but then include a value tRise in the formula that calculates the frequency lower, then you have to ask yourself why you calculate a frequency at all? What I want to say. If the connection (cable, pullups etc. with all considered) can't do the desired frequency, then you have to choose your frequency lower. If you change the period in the formula from the beginning you never get your desired frequency. I think this is understandable to everyone. :-) |
Beta Was this translation helpful? Give feedback.
-
|
Hi, Test in Excel your formula or mine and enter the calculated BAUD value in the terminal of my program to see how it clocks on the oscilloscope. /* mikrocoder, 29.01.2022
AVR128DB64 Dev Board
https://github.com/SpenceKonde/DxCore/discussions/243 */
template <class T>
void formatBIN (Stream &out, const T &var, const bool separator = false)
{
// Anzahl der Bits vom Datentyp T ermitteln und eins abziehen, //
// damit wird die Bit Startposition der Maske erzeugt und //
// die for Schleife limitiert //
constexpr uint8_t limit {(sizeof(T) * 8) - 1};
uint64_t mask {1}; // wegen Overflow Compilerwarnung aufgetrennt
mask = (mask << limit);
uint8_t digit {0};
out.print("0b");
for (uint8_t i = 0; i <= limit; i++)
{
if (var & mask) digit = 1;
else digit = 0;
out.print(digit);
if (separator)
{
// aller 8 Bits ein Hochkomma dazwischen schieben und den Letzten unterdrücken
if (!((i + 1) % 8) && (i < limit)) out.print("'");
// aller 4 Bits ein Punkt dazwischen schieben und das Letzte unterdrücken
else if (!((i + 1) % 4) && (i < limit)) out.print(".");
}
mask = mask >> 1;
}
}
template <class T>
void formatHEX (Stream &out, const T &var, const bool separator = false)
{
uint64_t data {0};
bool sig {false};
const uint8_t len {sizeof(var)};
if (var < 0) {
sig = true;
data = static_cast<uint64_t>(var * (-1));
}
else {
data = static_cast<uint64_t>(var);
}
uint8_t buf[len+1];
memset(buf, '\0', len);
// isolate the incoming data type into single bytes
for (uint8_t i=0; i < len; i++)
{
buf[i] = static_cast<uint8_t> (data & 0xFF);
data = (data >> 8);
}
if (sig) out.print('-');
out.print("0x");
for (int8_t i=len-1; i >= 0; i--)
{
// ggf. fehlende führende Null vom Datentyp Byte ausgeben
if (buf[i] <= 0x0F) out.print('0');
out.print(buf[i], HEX);
if (separator)
{
// aller 2 Datentyp Bytes ein Hochkomma dazwischen schieben und das Letzte unterdrücken
if ((!(i%2)) && (i > 0)) out.print("'");
}
}
}
#include <Wire.h>
const byte SERIAL_BUFFER_SIZE = 5;
char serialBuffer[SERIAL_BUFFER_SIZE];
const byte userLed0 {PIN_PB4};
Stream &cout {Serial2};
void setup()
{
Serial2.swap(1); // PF4 TXD2 / PF5 RXD2
Serial2.begin(500000);
cout.println("\nµC Reset #### ####");
pinMode(userLed0, OUTPUT);
//Wire.usePullups(); // extern 3,3k
Wire.begin();
//TWI0.CTRLA = 2; // Fast Mode Plus aktivieren
cout.println(F("bitte einen 8 Bit Wert für das BAUD Register eingeben"));
cout.println(F("please enter a 8 bit value for the BAUD register\n"));
}
void loop (void)
{
handleSerial(cout);
Wire.beginTransmission(0x68); // Example DS3231
Wire.endTransmission();
heartbeat(userLed0, 1000);
}
bool read_serial(Stream &cout)
{
static byte index = 0;
if (cout.available() > 0) {
char c = cout.read();
if (c == '\n') { // 'NL' Endeerkennung
serialBuffer[index] = '\0'; // Null Terminierung
index = 0;
return true;
}
else if (c >= 32 && index < SERIAL_BUFFER_SIZE - 1) { // alles lesen was kein Steuerzeichen ist
serialBuffer[index++] = c;
}
}
return false;
}
void handleSerial (Stream &cout)
{
if (read_serial(cout) ) {
int data = atoi(serialBuffer); // string to int
cout.print(F("eingelesener Wert: "));
cout.println(data);
if (data >= 0 && data < 256)
{
TWI0.MCTRLA = 0;
TWI0.MBAUD = static_cast<uint8_t>(data);
TWI0.MSTATUS = 1;
TWI0.MCTRLA = 1;
showTWIRegister(cout, 0);
memset(serialBuffer, '\0', sizeof(serialBuffer)); // seriellen Buffer löschen
}
}
}
void showTWIRegister (Stream &out, const unsigned long interval)
{
static unsigned long lastMillis {0};
if (millis() - lastMillis >= interval)
{
lastMillis += interval;
out.print("TWI0:");
out.print("\nCTRLA: "); formatBIN(out, TWI0.CTRLA, true); out.print(" "); formatHEX(out, (uint16_t)&TWI0.CTRLA);
out.print("\nMCTRLA: "); formatBIN(out, TWI0.MCTRLA, true); out.print(" "); formatHEX(out, (uint16_t)&TWI0.MCTRLA);
out.print("\nMSTATUS: "); formatBIN(out, TWI0.MSTATUS, true); out.print(" "); formatHEX(out, (uint16_t)&TWI0.MSTATUS);
out.print("\nMBAUD: "); formatBIN(out, TWI0.MBAUD, true); out.print(" "); formatHEX(out, (uint16_t)&TWI0.MBAUD); out.print(" "); out.println(TWI0.MBAUD);
out.println();
}
}
void heartbeat (const byte pin, const unsigned long interval)
{
static unsigned long lastMillis {0};
static bool state {LOW};
if (millis() - lastMillis >= interval) {
lastMillis += interval;
state = !state;
digitalWrite(pin, state);
}
}
|
Beta Was this translation helpful? Give feedback.
-
|
I've set up an AVR128DA48 (16MHz int. Osc) in loopback mode (I enabled the Master and Slave on the same pins) with an external 10k Poti to be able to test the clocks. first of all, there was a difference of about 50kHz on the internal measurment function of my osciloscope just by reducing the Pullup from 3.3k to 1k. So don't bother with like +/-5% frequency deviation. Second,
from the Datasheet:
This means you Initialized your TWI wrong and thus could probably not use Value 1 (you have to set MSTATUS after MCTRLA). But I have to admit that I had only 500kHz with 3.3k Pullup (550kHz with 1k) on default. |
Beta Was this translation helpful? Give feedback.
-
|
I have amended the Wire library clearer about this behavior,. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, Thanks for the MSTATUS after MCTRLA hint. Because of the frequencies. Also with 1,6k pullup (2x 3,3k parallel) it doesn't really get better.I think this is because there are 5 TWI devices connected onBoard on the bus. The load will be too high. I have to check this again with "empty" bus. Where you have to be careful with low pullups that it does not raise the low level. 3,3k > 340kHz First of all, thanks for the conversation. If there is something new I will contact you again. Translated with www.DeepL.com/Translator (free version) 1.6k Pullup, Wire.setClock(400000) |
Beta Was this translation helpful? Give feedback.
-
|
I measured this again with 1k pullup. I still do not reach tRise smaller than 300ns. Nevertheless the clock signal looks perfect. But you can also see that it raises the low level by 360mV. That should worry one with low pullup values more than keeping the rise-time. Because 1k is already very low. https://www.gammon.com.au/forum/?id=10896&reply=5#reply5 Even with the onboard devise disconnected and only one external, it doesn't get much better. Practically not relevant. Did you measure this with real I2C device and look at all parameters? I wonder at the moment how to reach frequencies higher than 400kHz if I can't reach 400kHz according to the specification. 800kHz are pure theory with that? Or the device may be positioned only 1mm next to the controller pin. My current opinion. You can do what you want with the formula. If you want to have it exactly you have to measure and look at your signal. There are too many factors that influence everything. You can't adjust that with a formula. Otherwise one has perhaps according to measurement the desired clock, which is however so ground that possibly the bus does not function any longer cleanly. And with the next user everything looks completely different, because other board, other cable lengths etc.. 1k Pullup, Wire.setClock(400000) |
Beta Was this translation helpful? Give feedback.
-
|
Did you know that the I2C pins have an Fast Mode Plus setting? This should increase the sink current in order to compensate for the smaller resistor. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, Wire.setClock(400000) does not switch on the FastModePlus yet. You could switch on the FMP always at > 100kHz? Or not? |
Beta Was this translation helpful? Give feedback.
-
|
It is probably possible. The only downside would be the (slightly) increased power consumption. The Core enables FMP at >=600kHz. |
Beta Was this translation helpful? Give feedback.
-
|
400 kHz should be FM, not FMP.... Have you tried using the input level option (in TWIn.CTRLA to set it to use the SMBus voltage levels? That has a lower HIGH threshold, and might end up working better.... |
Beta Was this translation helpful? Give feedback.
-
|
One could argue that at anything above 400kHz , it should be FMP mode. Maybe that's what we should be doing. |
Beta Was this translation helpful? Give feedback.
-
|
Probalby. I remember, when rewriting the library, I was not sure if anything above 400kHz counts as FMP and below FM, or anything above 1MHz is FMP and below is FM |
Beta Was this translation helpful? Give feedback.
-
|
Okay, I was just asking. :-) Because of the mode. Everything is ok. :-) |
Beta Was this translation helpful? Give feedback.
-
|
I have now measured this again with the usual 2.2k pullup. I don't like the frequency deviation, but there is little you can do about it. I will limit myself to 400kHz. Even if even there already 13% are missing. One must be able to live with it. :-) Again the comparison with standard and FMP. The FMP does not have much effect. Actually nothing. Maybe only useful with very small pullups. With which you raise the low level, which is also not nice. I hereby end this for me. Thanks for the conversation. set 400kHz set 400kHz FMP set 600kHz set 600kHz FMP set 800kHz set 800kHz FMP |
Beta Was this translation helpful? Give feedback.
-
|
just as a side note, you could just make your own offset - if 400kHz result in 350kHz, just add the missing 50 to the original function call. And I think you are way too focused on the low-level voltage, after all - again taken from the Electrical Specifications, the lowLevel >400kHz is below 0.2xVDD - thus 0.2 * 5V = 1V. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, whether you like it or not, you need to readjust. I repeated the measurements with 10:1 probe and the curves look much better. Yesterday I had a 1:1 probe. If the BAUD values drift away with increasing frequency, then you need to fine tune. If with 800kHz 100kHz are missing, then actually too much. I don't care about a few kHz deviations, they are normal. But the big differences should not be. Furthermore FMP should be active at all frequencies higher than 400kHz. Not only from 600kHz. Because I think that you have the claim to make the Wire Lib better. Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
I know. The problem is though, that the BAUD relies on the F_CPU, Resistor and capacitance. And existing libraries are using only one argument in the setClock function. This means, that the library has to offer the user some sort of calibration, while being 'good enough' that it works without calibration. If possible, the algorithm should be as small as possible. It is on my ToDo list though |
Beta Was this translation helpful? Give feedback.
-
|
I wouldn't put a lot of effort into it. You could overload the setClock function. Only it doesn't make sense according to my opinion. And why the same for all controllers? We are here in the Wire Lib in the DxCore. Why does the algorithm have to be small? The clock is not changed constantly ;-) I would assume a good bus state and look for the appropriate parameters or a formula for it. If then the user uses the wrong pullups, or too long lines, then that's his problem. I want nothing more than that the initial state is correct. Big topic I know. If you don't have time, then so be it. Other question. I have extended my test program so that I do not always have to flash and can enter everything via the terminal. If I leave out the marked two lines, then actually only the SCL should not clock anymore, because the master is switched off. But my controller blocks completely. My heartbeat led does not blink anymore. What can it be that TWI blocks the controller? void setBaudRegister (Stream &cout)
{
if (baudSwitch.newData)
{
TWI0.MCTRLA = 0;
if (baudSwitch.data == 0) {
baudSwitch.trigger = !baudSwitch.trigger;
if (baudSwitch.trigger) { TWI0.CTRLA = 2; }
else { TWI0.CTRLA = 0; }
TWI0.MCTRLA = 1;
TWI0.MSTATUS = 1;
showTWIRegister(cout, 0);
}
if (0 < baudSwitch.data && baudSwitch.data < 256) {
TWI0.MBAUD = static_cast<uint8_t>(baudSwitch.data);
TWI0.MCTRLA = 1;
TWI0.MSTATUS = 1;
showTWIRegister(cout, 0);
}
if (baudSwitch.data > 255) {
Wire.setClock(static_cast<uint32_t>(baudSwitch.data));
>> TWI0.MCTRLA = 1; <<
>> TWI0.MSTATUS = 1; <<
showTWIRegister(cout, 0);
}
baudSwitch.newData = false;
}
}Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
Belive it or not, the megaTinyCore and the DxCore are using the same Wire library. It significantly improves maintainability
Graphical Libraries like u8g2 and AdafruitGFX are calling setClock before every transfer
I cannot follow this question. The current implementation calculates the BAUD, if it differs from the current BAUD, saves the MCTRLA register, sets MCTRLA to 0, sets the new BAUD, restores the MCTRLA and sets MSTATUS to IDLE. I think I see the problem. If I remember correctly, the MSTATUS can be set, even though MCTRLA is not set. When you call endTransmission, it is only checked, if MSTATUS is UNKNOWN. There is a timeout, but it is reset if the MSTATUS is IDLE, because at the same time MADDR is written and the code expects the TWI to be enabled and thus the state machine to change... Basically a bug that happens in the setClock. IDLE should be only set if MCTRLA & 0x01 |
Beta Was this translation helpful? Give feedback.
-
|
You have understood my question correctly. In any case, your answer explained everything I wanted to know. :-) void setBaudRegister (Stream &cout)
{
if (baudSwitch.newData)
{
TWI0.MCTRLA = TWI0.MCTRLA & ~_BV(0);
if (baudSwitch.data == 0) {
baudSwitch.trigger = !baudSwitch.trigger;
if (baudSwitch.trigger) { TWI0.CTRLA = TWI0.CTRLA | _BV(1); } // FMP on
else { TWI0.CTRLA = TWI0.CTRLA & ~_BV(1); } // FMP off
}
if (0 < baudSwitch.data && baudSwitch.data < 256) {
TWI0.MBAUD = static_cast<uint8_t>(baudSwitch.data);
}
if (baudSwitch.data > 255) {
Wire.setClock(static_cast<uint32_t>(baudSwitch.data));
}
TWI0.MCTRLA = TWI0.MCTRLA | _BV(0);
TWI0.MSTATUS = TWI0.MSTATUS | _BV(0);
baudSwitch.newData = false;
showTWIRegister(cout, 0);
}
}Because of the BAUD calculation. I don't know if this is possible with respect to compatibility calculations or parts of it to be calculated at compile time. Do you program only C or also C++? May I also ask why you exchanged a ring buffer for a linear buffer? A ring would have more advantages. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, I have not forgotten you. I am still programming and testing. 😉 |
Beta Was this translation helpful? Give feedback.
-
|
Hi, actually I wanted to change the lib to baudrate calculation at compiletime. This also works when I test the code individually. If I build the code into the twi lib, it has a completely negative effect. The flash occupancy increases instead of decreasing. Would have been too nice. 😥 The measurements. From my point of view the change was successful. The frequencies 300kHz, 500kHz and 500kHz should not be overestimated. Nobody will use these frequencies. 100, 200, 400, 800kHz, 1MHz can be used quite comfortably with 4.7k, 3.3k and 2.7k. This is all to full satisfaction. 👍 👏 Thanks. Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
|
Hi, there is one more thing. The baud register must not be 0. With 0 nothing clocks. I would write the TWI_Baud formula a little bit different. First multiplication then division. Increases the accuracy in extreme cases because of integer calculation. So that one does not fly out of the uint32 value range in the extreme case an extension on uint64. Hopefully you have defined F_CPU with xUL? 😉 Suggestion. #define TWI_BAUD(freq, tRise) ( (F_CPU / 2 / freq) - ((1ULL * F_CPU * tRise / 2000 / 1000000)+5) )
uint8_t TWI_MasterCalcBaud(uint32_t frequency)
{
int32_t baud = 255;
if (frequency == 0) { // division 0
frequency = 1;
}
#if (F_CPU == 20000000) || (F_CPU == 10000000)
if (frequency >= 600000) { // assuming 1.5kOhm
baud = TWI_BAUD(frequency, 250);
} else if (frequency >= 400000) { // assuming 2.2kOhm
baud = TWI_BAUD(frequency, 350);
} else { // assuming 4.7kOhm
baud = TWI_BAUD(frequency, 600); // 300kHz will be off at 10MHz. Trade-off between size and accuracy
}
#else
if (frequency >= 600000) { // assuming 1.5kOhm
baud = TWI_BAUD(frequency, 250);
} else if (frequency >= 400000) { // assuming 2.2kOhm
baud = TWI_BAUD(frequency, 400);
} else { // assuming 4.7kOhm
baud = TWI_BAUD(frequency, 600);
}
#endif
return baud < 1 ? 1 : (baud > 255 ? 255 : baud);
}
} |
Beta Was this translation helpful? Give feedback.
-
Are you sure? Is it stated somewhere in the datasheet? I was able to get 1MHz fSCL with 16MHz F_CPU only by setting the baud register to 0. And it was working fine. |
Beta Was this translation helpful? Give feedback.
-
|
The data sheet does not say anything about this. In all tests with your code the baud register is not set to 0, it is set minimum to 1. If I set the register itself to 0, nothing clocks anymore. I had noticed this weeks ago. I don't know why this is. I tested it again today. It seems to work randomly and depends on the CPU clock and its setting. |
Beta Was this translation helpful? Give feedback.
-
|
Ok, so baud 0 does not work with 16MHz crystal... Can you get it to work with the internal 16MHz? I was using the DA48, so maybe they changed something on the silicon? |
Beta Was this translation helpful? Give feedback.
-
|
AVR128DB64 internal 8MHz: everything okay Somehow there seems to be a problem. It's crazy. |
Beta Was this translation helpful? Give feedback.
-
|
I have another explanation.I always test with real device on the bus. |
Beta Was this translation helpful? Give feedback.
-
|
I just run some test myself again. I was using the Master and Slave functionaliy of the AVR on the same pins to make sure to have a slave that is specified to work at 1MHz fSCL. I think I messed up with the 16MHz CPU, since it was not working on my end either. But I think I remember doing this for F_CPU <=10MHz to get the highest possible frequency. |
Beta Was this translation helpful? Give feedback.
-
|
basically, |
Beta Was this translation helpful? Give feedback.
-
|
Hello, yes okay, you can do so. But you certainly mean it that way. So there is no gap in the frequency range. 😉 #if (F_CPU > 16000000)
const uint8_t baudlimit = 2;
#elif ((F_CPU <= 16000000) && (F_CPU > 12000000))
const uint8_t baudlimit = 1;
#elif (F_CPU <= 12000000)
const uint8_t baudlimit = 0;
#endif
if (baud < baudlimit) {
baud = baudlimit;
} else if (baud > 255) {
baud = 255;
}
return (uint8_t)baud; or 😉 return baud < baudlimit ? baudlimit : (baud > 255 ? 255 : baud);And please make baud int32_t, otherwise there will be uncontrolled overflows which will be valid then by chance in int16_t. It can be really unfavorable if the int16 value is in the uint8 range and thus baud is evaluated incorrectly. Calculate once the possible extreme values. Then you know what I mean. You can also use the values of tRise directly. I would not shorten them by /10. Whether you change the formula or the tRise value doesn't matter. And please prevent the division 0 of frequency. |
Beta Was this translation helpful? Give feedback.
-
|
The gap is there, because at 8MHz and 4MHz the SCL freq. was more inaccurate with 0 then with 1 as limit, so I just limited it there.
I try to avoid theese, because with more then one condition it becomes harder to read then when it is splitted. Sometimes it makes the debugging harder too.
I've come to the realization, that the substraction part ( (5 + (((F_CPU / 1000000) * t_rise) / 2000) ) is completely precalculated by the compiler, t_rise and F_CPU are known at compile time. Changing it won't really matter.
This will be done in the function before. |
Beta Was this translation helpful? Give feedback.
-
|
Hello, I don't see any prevention of division by 0. Where should it be? There are only 3 function calls in the context and the define formula. void TwoWire::setClock(uint32_t clock)
void TWI_MasterSetBaud(struct twiData *_data, uint32_t frequency)
uint8_t TWI_MasterCalcBaud(uint32_t frequency)I also still see the problem with int16_t baud. It must be int32_t. Otherwise it comes to implicit cast which end up uncontrolled in the value range 0-255, although they have to be limited to 0 or 255. The result of TWI_BAUD may exceed the value range of int16_t. Then implicit overflows occur and thus uncontrolled results. E.g. look at this in a smaller form what happens wrong. void setup()
{
Serial.begin(9600);
for (int16_t i = -1000; i<1000; i=i+10)
{
Serial.print(i);
Serial.print(" ");
Serial.println((uint8_t)i);
}
}
void loop()
{ }Just as wrong happens with the return value TWI_BAUD when it is squeezed into an int16_t. |
Beta Was this translation helpful? Give feedback.
-
|
the check for 0 is in MasterSetBaud. Since the AVR does not support hardware division, nothing bad will happen if 0 is actually supplied. and about getting int32_t - I just don't see any reason why we should use 24 more bytes for the calculations - afterall, to get baud to overflow on int16_t, the user would have supply a value below 400Hz (at 24MHz CPU) to the function. A frequency that is not supported by the TWI to begin with (at this CPU clockspeeds). |
Beta Was this translation helpful? Give feedback.
-
|
The optimizer is smart enough that using ternaries for the return doesn't buy you anything and just makes the code harder to read. I agree with MX682X if you would need to supply such an extreme value to generate an overflow in an intermediate. This code gets ported to megaTinyCore, where it will be used on parts with as little as 2k of flash, and pincounts where the largest flash available is only 4k. An overflow that can occur when the function is passed normal reasonable values? That's a problem. As MX682X points out, this doesn't happen when the value passed in is a little bit wrong, it happens when they ask for a frequency that is 1/256th the lowest that the hardware can do. At that point, setting it to 255 is still giving the user a SCL clock ~256 times faster than they asked for! And only a calculated value between 65535 and 65791 would manifest the bug... After that it would be another 65k before it yielded a number that didn't end up at the maximum baud value and hence lowest speed In an ideal world, we'd be able to at least catch invalid constant frequencies passed to it it, but this is "shielded" by a class, and classes are kryptonite to the optimizer. Even when a method is called in a single place, with an integer literal argument, the compiler doesn't seem to be able to perform constant folding, and hence the error detection methods that we usually use don't work. Mikrocoder, if you have examples of reasonable values that someone might pass to this function that will result in overflows please do share them. Looking at that, I think it would take hard work to find a value that would cause a detectable overflow. |
Beta Was this translation helpful? Give feedback.
-
|
Hello, I must be more clear. I am beginning to doubt the confidence in the DxCore when I have to read such answers. Sorry. Whether the controller supports Division in hardware or not doesn't matter. If not, it will be emulated in software. However, division by 0 is not allowed. You may assume that nothing happens. You can't guarantee it! Where is in MasterSetBaud() the check for 0? Show me the line. And how do you get another 24 bytes because of int32? That's 2 bytes more. But they are local and therefore temporary and will be removed after the function is finished. So it doesn't matter. If too low or too high frequencies are supported is also irrelevant. Exactly this is intercepted with the limitation to 0 - 255. But for this it requires before a correct computation without implicit cast overflow error. Examples: F_CPU 48MHz F_CPU 48MHz Everything outside of the int16_t value range. Here it may not come to a wrong cast, because exactly these values must be seized with the limitation between 0 - 255. The user will not notice anything, he is only happy that it works. Only when he remeasures, he may notice that the clock is higher than desired. You have to program more defensively. You see only what you want to see. Not what the user could do. You have to ask yourself what kind of crap the user can enter. The programming has to be designed for that. I like to repeat myself again. You have to think about what a user can do with your program. You must not only think about what you would do with it. Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
|
We are not talking about bytes of ram for intermediate values nobody cares about that. We are talking about bytes of flash. The instructions to manipulate an larger variable add up, frequently several times as much as the larger datartpe adds to the memory footprint, and like I said, this same library code is used on parts with 2k of flash. Over there, anything I do that increases flash size, people scream over. The DD-series will extend DxCore down to 16k parts and the EA-series to 8k, so you don't even need to look outside the core to find flash constrained chips which will use this code. And all we could do better handling is give them a speed a few hundred times the speed they requested instead of a few thousand Either way, we can't give them anything close to what they asked for, and I'm unconvinced that It's even helpful to the users who encounter it. |
Beta Was this translation helpful? Give feedback.
-
|
Basic question. Are you looking at the Flash alone? You don't care whether the code is programmed cleanly? Is that your goal? You don't care that errors are not caught? Sorry, but this is the completely wrong approach. First of all the functionality should be ensured. After that it can be optimized. You optimize first and don't want clean code. My trust in the DxCore does not get better by that. This is where different points of view meet. Again. Someone could set 1Hz. By wrong implicit cast could be 0 in baud register in worst case. The user wonders why his I2C does not work, although he has set it to the slowest clock. Here the 255 limit would have to take effect. But it can't. You don't care at all? I can hardly believe it. Don't you want to understand the problem? Or can you not understand the problem? That is the crucial question. And don't answer with flash, flash, flash again. If you were really concerned about the Flash, then instead of if (baud < 1) {
baud = 1;
} else if (baud > 255) {
baud = 255;
}
return (uint8_t)baud;you would write that. return baud < 1 ? 1 : (baud > 255 ? 255 : baud);
or with your new code
return baud < baudlimit ? baudlimit : (baud > 255 ? 255 : baud);Because that saves a few bytes of flash. But that's not what you want. Why? I think you want to save flash? Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
You are free to not use the code if you don't like it - you can always make a fork with your additions
good catch. by directly returning the 255 or baudlimit, the 2 byte wide baud variable does not need to set (thus saving one instructions each). Fixed that now. I have following suggestion how to handle the low baud problem: If the user supplies a frequency lower then F_CPU/1000 (resulting in a minimum frequency of 1000 - 4800 Hz SCL (hardware min is higher, about 1900 Hz (calculated) at 1MHz CPU) ), the function will return 1, a vlaue indicating a error in the Arduino API, otherwise 0. When the frequency is constant at compile time, the compiler will issue an error. This will solve the problem of 0 and low frequencies that will result in the overflow of the baud variable. Also, if there is no check for the return, it is optimized away and the function just does nothing in case of a too low frequency. |
Beta Was this translation helpful? Give feedback.
-
|
I mean, if it were possible to catch constants (since 99.9% of the time what will be passed will be a constant) and react to them, yeah that would be great. |
Beta Was this translation helpful? Give feedback.
-
|
Oh, please don't forget to mention this problem to the Arduino team, they use this for their calculations. For a couple of years, noone seems to have trouble with it....
|
Beta Was this translation helpful? Give feedback.
-
|
Isn't it funny? The official Arduino core does shit like that and goes years without complaints. On the other hand, this is what, the 4th reimplementation of this specific function over here? Each of which was somewhat less wrong than the one that came before it.... and every one of them prompted by complaints from users. MCUDude of MegaCoreX also said he has never been asked for support for XDIR/RS485 mode (I'd gotten at least 3 requests, maybe 4, and MegaCoreX predates my cores). I think DxCore and megaTinyCore tend to take a disproportionate share of the most skilled users, and so we end up with more people doing exotic things and demanding fixes to make said things work. |
Beta Was this translation helpful? Give feedback.
-
|
Do you always have to create forks right away? I don't think forks make sense. Because nothing gets better from it. It just makes everything more complicated. I only want to contribute ideas and improvements. Even if there are errors in the Arduino code, it does not mean that you have to keep them. And you mean F_CPU/1000 (resulting in a minimum frequency of 1000 - 48000 Hz SCL ... |
Beta Was this translation helpful? Give feedback.
-
|
Hi, please You should not compare anything with the Arduino Core. The Arduino has enough bugs that are not fixed. Want an example? The SPI constructor has a bug. This happens to work with the avr-gcc 7.3.0 toolchain. From version avr-gcc 10 on SPI does not work anymore, because it optimizes the constructor away. Has been reported but no one cares. My last objections here in the DxCore concern only the security that the code does what it should. Direct program errors are not. Only I wanted to address the problems so that afterwards nobody is surprised. If I program the whole baud calculation with C++ it optimizes everything away at compile time. Exactly as I imagine it. Unfortunately this works in the Wire/twi Lib only partially. This is also kind of normal due to the separation in .h and c you have only limited possibilities. Even if I change this to .h and .cpp I have to live with limitations. I have already tried many things. The new variant still generates 12 bytes too much. Maybe there is another possibility. Everything else would require a completely new lib. But I don't start that now. And because of the MegaCoreX. Maybe many switch to the DxCore before they have tried everything with their controllers. I think the Arduino Nano Every is wonderful. Unfortunately there are too few pins leading out which makes it heavily neutered. Even though you can "re-route" a lot with the event system. It does not become therefore more pins. That's why I changed to the AVR DB. And MCUdude can take over your changes if he wants. I leave it for now. I don't want to annoy you too much. 😄 Thanks for all the changes you have made. Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
Hi,
I would also like to contribute something. :-)
It is about the documented deviation between target and actual TWI frequency. The parameter tLow makes this all mixed up. A higher frequency has now once the consequence that tLow becomes smaller. Why Arduino has taken over this in their formula I do not understand. I have me therefore own thoughts makes and come on the following formula the BAUD suitably computes.
BAUD = (F_CPU[kHz] / fSCL[kHz] / 2) - (5 + (F_CPU[kHz] / 1000 / 4))(Excel floating point calculation)fSCL in kHz should be sufficient.
F_CPU in kHz is formula conditioned, can be adapted if necessary in the function and would have also otherwise enough accuracy for crooked values like 3.333MHz. It occurs to me that this CPU clock is still missing in your menu setting, because this is the standard clock without further register change.
The result should be rounded mathematically correct (+0.5) and all is good. Or without rounding and you have to calculate with a deviation which should not be a problem. Is still worlds more accurate as Arduino does it. The possible maximum clock is of course always dependent on the clock with which you fire the controller. With usual 16MHz only 600kHz are possible.
I calculated this in Excel, compared and tried the BAUD values and remeasured. Fits everything very well.
I found out that in the standard mode TWI0.CTRLA no BAUD values between 0 and 2 may be passed. So no clock is output. Is anything known about this?
Any opinions?
Translated with www.DeepL.com/Translator (free version)
Beta Was this translation helpful? Give feedback.
All reactions