Closed SpenceKonde closed 2 years ago
1.5.0 at the latest. Recent events underlined the problems with the second core. It's a steaming pile of garbage, and calling it "garbage" is being polite.
Which two cores? I'm confused.
Take a look in avr/cores..... there are two COMPLETELY SEPARATE CORES in ATTinyCore!!! It used to be two packages, but I papered over the differences with board menus and similar behavior. Also notice how only one of them uses variants. I'm most of the way done with combining them properly, but then I saw something boneheaded in the ADC code where we're wasting approx 24 bytes of flash and 30 (or more) clock cycles at runtime computing something that could be pre-calculated and rolled into the analog reference constants. Look at the "modern" one if you want to see what happens when someone is too clever with preprocessor macros for their own good. And everyone else's good, for that matter (see, the origin of these cores was I found someone who had done part of the job of supporting the 841, took it and finished it, then added the 1634 and 828 to it. Then wanted to use the other tinies, and found cores for them, but those cores used variants and were built very differently. I wasn't good enough yet to convert them back then in a reasonable length of time).
Ah. I thought you were talking about combining ATTinyCore and megaTinyCore!
My view: if it's just a case of achieving internal tidiness, but doesn't change the usability, then it's not worth the effort. But perhaps there's a consideration that it'll be easier to maintain once you've done it.
Mmmmm it was basically impossible to maintain at this point. Everything had to be done twice, there were no variant files defined (it was ifdefs scattered through half a dozen files, just utterly awful - all awash in "look how clever I am with macros" nonsense. Part of what prompted it was a bunch of changes that often didn't even need to be made to the normal core that were a BFD to implement on the other one. Plus we have the millis/micros fixes (those only went into the "tiny" core, not "tiny_modern". And the compile-time-known performance improvements for digital I/O... And now the ADC features are being exposed, and one of the most feature-rich ADCs is on the 841, which is a "modern" tiny.... If I had to keep them separate, I would not have done anything more on this core. This is likely the final hurrah for the classic AVR parts - get the core into a state where I'm not embarrassed by it, you know? in it's 1.5.0 state, it is frankly kind of a disgrace! and pre-1.4.0 it was unspeakably bad... This means that I want to get rid of decisions that I made that I knew were bad but did anyway because I didn't know how to do it right, knew that, but needed to do it somehow anyway, and so did it wrong. Speaking of "doing it wrong" - watch out with bitshifts:
I was doing:
ADMUX = ((analog_reference & ADMUX_REFS_MASK) << REFS0) | ((pin & ADMUX_MUX_MASK) << MUX0); //select the channel and reference
REFS0 was usually 6. Do you know what not_compile_time_known_uint8_t << 6
compiles to?
Well, the variable is promoted to an int16_t, and then it loads 6 to a register... and then loops over it leftshifting it once per pass through the loop and decrementing the counter... shifting the reference constants at compiletime saves ~30 clocks and ~24 bytes of flash.
Oh... and why, pray tell, do we get the ADC value by reading the high and low registers separately? when the io headers provide ADCW that uses it directly? With the code the core used (which I think is same in official cores) the compiler does one of the most braindead things I've ever seen it do (and that's really saying something).: It needs to get the two bytes into registers r24 and r25. Easy, just read the two bytes in right? No matter which order they were read in the code, the compiler would read them into the wrong registrars.... and then use three eor (exclusive or) instructions to exchange the values of the two bytes, when using ADCW, the compiler gets right straight off the bat. Why did they do it the awkward, stupid way? I suspect they didn't know there was a word register defined >.>
It's going to be 2.0.0 because I'm knowingly breaking backwards compatibility with sketches that used raw numbers to refer to analog channels (it's ADC_CH(channel number) or the An
define,, or the digital pin number now. It will break anyone who passes raw numbers to analogReference() (who the hell does that? They're being fools anyway if they do).... the old PIN_xn defines are getting axed and replaced with PIN_Pxn (as my other cores do), and PIN_An (digital-pin-on-channel-n - like official cores do)......
Sounds like the work will be worth it, but it's important it doesn't break old projects, otherwise I (and many other bloggers) will have to go back over all my old blog projects and rewrite them (I've got ATtiny projects going back to 2014).
breaking backwards compatibility with sketches that used raw numbers to refer to analog channels
I don't quite understand this. Can you give some examples; eg what's OK, and what will break?
In previous versions of the core, analogRead(0) was the same as analogRead(A0), that is, it would read ADC channel 0.
In 2.0.0 (and I think the combine-the-cores branch, for the parts it works on), analogRead(0) will read the analog channel associated with digital pin 0, (and A0 is defined as ( 0 | 0x80) - the I/O functions all know to strip off the high bit and if it's set, that the "pin number" they are being passed is an analog channel number not a digital pin number).
The handling of analog pins was a colossal blunder on my part, the largest mistake I made on the cores by far.
Good - I don't think that will break anything.
Oh - and referring to references by numbers instead of the constants will also break (I am pre-shifting the REFS bits into the place where they will end up at compiletime, so at runtime, it just needs to bitwise and/or stuff
So what's allowed. For example, on ATtiny85 which of these:
int n = 4; analogRead(4); analogRead(n); analogRead(A2); analogRead(PB4);
Few people will praise you for making the core run a bit faster, but plenty will curse you if their projects break.
~Can't you do something like: if it's a constant use efficient method, otherwise use old method?~ Ignore this
analogRead(4);
analogRead(n);
analogRead(PB4);
MUX selected is binary 0100 (invalid for single ended on Tiny85, see datasheet page 135)
analogRead(A2);
MUX selected is 0010 (high bit is stripped in analogRead
)
When using analogRead
to read from a pin, you should always use the Ax
constant to avoid doubt. Only pass an integer when you want to read from one of the non-pin analog sources (see datasheet page 135 for T85 ones for example).
So the only one that will work correctly is analogRead(A2)?
That will break my uLisp Lisp interpreter, which calls analogRead with the Arduino pin number of the analog pin you want to use, and which works fine on every other Arduino platform.
If you want to analogRead
from a pin marked A2 on the ATTInyCore pinout diagram, then that is correct, you should use A2.
Now, and in the past, and I'm sure @SpenceKonde does not intend to change that in the future.
Of course you can assign the value of A2 to a variable, because, that's how C works,
It has worked in the past, and works on every other Arduino platform as far as I know.
Of course you can assign the value of A2 to a variable, because, that's how C works
No I can't, because it's in my Lisp interpreter.
Here's the code of the Lisp analogread function (slightly simplified) to implement the Lisp function:
(analogread pin)
where pin is an integer containing the Arduino pin number:
object *fn_analogread (object *args, object *env) {
(void) env;
int pin;
object *arg = first(args);
pin = checkinteger(ANALOGREAD, arg);
return number(analogRead(pin));
}
How do I change that to work after this change? My only option will be to tell users to compile it under an older version of the ATTinyCore.
1.5.0 (I think)
int n = 4;
analogRead(4); // There is no ADC4. So what does ADMUX do when you do ADC4? It does a differential reading between A2 and itself. I think that's what you get *now*.
analogRead(n); //as above
analogRead(A2); // Reads ADC2 / arduino pin 4.
analogRead(2); // Reads ADC2 / arduino pin 4.
analogRead(PB4); // is always wrong!
Pxn constants where x and n correspond to a port and pin in that port that exists ARE ALWAYS DEFINED BY THE IO HEADERS TO BE THE POSITION OF THAT BIT. Yes, this is f---ing stupid. So it's the same as analogRead(4) but on other parts, PA4 is the same as PB4 is the same as PC4. Ask ATMEL/Microchip why it was done that way..... That's why the pin-by-port-and-bit defines my core provides are PIN_Pxn
not Pxn
. - it's taken and trying to undef styuff from the io headers is begging for trouble.
2.0.0
int n = 4;
analogRead(4); // This is not an analog channel (4 & 0x80 == 0), so we call digitalPinToAnalogInput() to find the analog channel to read from digital pin 4, that's adc channel 2,. Reads ADC2/ pin4. .
analogRead(n); //as above
analogRead(A2); // A2 is defined as ADC_CH(2) or (2 | 0x80) so we strip the high bit and read adc channel 2 (
analogRead(2); // same logic as analogRead(4) - will read digital pin 2 (which happens to be ADC1)
analogRead(PIN_PB4); // PIN_PB4 is defined as the pin number that is on PB4, digital pin 4, proceeds as above.
This can't be working correctly now in 1.5.0 - or ever, I don't think, unless I had some one-way hackjob in place, but I don't think I did,,,,
It has worked in the past,
If it has then only by coincidental good fortune because an integer passed to analogRead was always MUX
Per @SpenceKonde's 2.0.0 plan he outlined just now, the change will actually benefit you, that is, make it do what you thought it was doing (but wasn't).
It will break other people's code, but yours will not be broken any more, I guess.
@SpenceKonde what if people want to read from some other adc channel, I guess they will use ADC_CH
macro?
So my code has only ever worked correctly on platforms where analogRead(Ax) and analogRead(x) happen to be equivalent?
Uhm, yeah that would be my assessment.... And that s the kind of counterintuitive nonsense that made me hate the way the old core was set up. I inherited t, and made poor decisions on top of that.... but i wasn't a total fool with how I assigned pin numbers. So often the adc channels match the digital pin numbers (I think they do on the 84, with one of the mappings at least....
But I read this statement in several places; this is from your Dx core ReadMe:
The An names are intentionally not shown on the pinout charts, as this is a deprecated way of referring to pins.
It will break code that passed analog channel numbers as numbers, rather than An constants. I also think that the misconception about analogRead() and digital pin numbers was VERY widespread..... This should have been done ages ago, frankly
Yes, DxCore and megaTinyCore did it the right way from the start, and want digital pin numbers passed to analogRead(). For the persistent individuals who look up the analog channel numbers in the datasheet, An is just defined as thje digital pin number that that analog input s on, and thee s no legacy history of An constants and "analog pin numbers" in people's heads
I also think that the misconception about analogRead() and digital pin numbers was VERY widespread
If something is a widespread misunderstanding then it has effectively become a fact, because enforcing the "correct" way of doing it will break many people's code.
Some other considerations:
Arduino users shouldn't have to know about MUX numbers, so being able to pass a MUX number to analogRead(n) is not useful. They might as well do ADCMUX = n.
So if you pass a number to analogRead(n) the only sensible thing for it to be interpreted as is an Arduino pin number.
As far as I know, calling analogRead(pin) with the Arduino pin number works on every other platform I've tried (ATmega328P, ATmega1284, ESP32, ATSAMD21, ATSAMD51).
I agree they shouldn't have to know MUX numbers (which is what analog channel numbers are). Which is why I am making analog read take arduino pin numbers and do what a normal person would expect. Of course have to accept An constants because that's what the people who did it like they were supposed to did, that's what the official arduino boards want you to do - and hence the high bit trick, since I get to define the An constants.
Arduino does unholy things on the official core that doesn't translate to all, or most, parts. Try analogRead(0) on an uno. It's reading same as analogRead(A0)! An constants are defined as the same number as the digital pin number, but the core subtracts the digital pin nuumber of the first analog pin from the pin passed to analogRead ifi it's larger than that number, to get channel...... That's the boneheaded "thinking" that got us into that mess.....
I think most other cores had conveged on the only sane behavior sooner that ATTinyCore. In any event, sanity is coming
Try analogRead(0) on an uno. It's reading same as analogRead(A0)!
No, analogRead(A0) is the same as analogRead(14). From pins_arduino.h:
#define PIN_A0 (14)
#define PIN_A1 (15)
#define PIN_A2 (16)
#define PIN_A3 (17)
#define PIN_A4 (18)
#define PIN_A5 (19)
#define PIN_A6 (20)
#define PIN_A7 (21)
static const uint8_t A0 = PIN_A0;
static const uint8_t A1 = PIN_A1;
static const uint8_t A2 = PIN_A2;
static const uint8_t A3 = PIN_A3;
static const uint8_t A4 = PIN_A4;
static const uint8_t A5 = PIN_A5;
static const uint8_t A6 = PIN_A6;
static const uint8_t A7 = PIN_A7;
Which is why I am making analog read take arduino pin numbers and do what a normal person would expect.
Great! So you are going to do what I'm saying is the sensible thing? On ATtiny85 Arduino pin 4 is A2 and is ADC2 (and MUX is 2), so in 2.0.0 these will be equivalent?
analogRead(4) analogRead(A2)
OK, in summary, the only thing that will break is if people thought they could call analogRead(n) with an integer specifying the channel number rather than the Arduino pin number? And you're saying that on the Uno they fudged it so that both would work?
That is correct.
Spence Konde Azzy’S Electronics
New products! Check them out at tindie.com/stores/DrAzzy GitHub: github.com/SpenceKonde ATTinyCore: Arduino support for almost every ATTiny microcontroller Contact: @.***
On Wed, Mar 17, 2021, 05:45 David Johnson-Davies @.***> wrote:
OK, in summary, the only thing that will break is if people thought they could call analogRead(n) with an integer specifying the channel number rather than the Arduino pin number? And you're saying that on the Uno they fudged it so that both would work?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SpenceKonde/ATTinyCore/issues/431#issuecomment-800943885, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTXEW64FSIFYQSSCVE2YJLTEB233ANCNFSM4PCGA7VA .
OK, sorry if I've prolonged this discussion by misunderstanding it.
As far as I'm concerned then it was wrong in 1.5.0 and you're fixing it in 2.0.0.
It can't always work that analogRead(n) will work with either the Arduino pin number or the channel number, as sometimes it will be ambiguous; it was just lucky that A0 was 14 on the Arduino, and there were only 6 ADC channels.
I've never seen it documented that analogRead will work with the channel number, but I have seen it documented that it will work with the Arduino pin number, so I believe you're fixing it the right way round.
This is now avaialble for public testing in the v2.0;0=dev branch.
The two cores are now merged. Issues with the resulting merged core should be reported in a new issue.
I don't think there is that much that is unique about any one board that would require that much conversion effort... We would use tiny, not tinymodern, as the tinymodern people were way too into metaprogramming, and nowhere near into variants. The code there looks huge, but I think most of it is macro-masturbation, and providing an API nobody knows about or uses for manipulating features that people who want to use would just achieve by manipulating bits like everywhere else does.
We'd pull in hardware serial with the support for more than one serial port (though the x7 LIN USART implementation would need to be pulled in and #ifdef'ed in to cover that, but that's obviously doable, and not even that hard). There's handling of the internal oscillator hack, obviously, but that's easy as well, and there's a clear place for it in the tiny implementation) There's a new init() subfunction to initialize timer2 on the 441/841 (just copy of that for timer1) And timer-mux initialization, too - current implementation is just writing a few constants to it And there's the PUENx register support to turn on pullups... - okay, 1 ifdef in two places? Their timers are well-behaved Is anything else different? Like, that should remain different? Is any of it so hard as to warrant keeping them apart, vs having to maintain two differently organized cores I don't think it's that much work - the tiny core does things right generally, and the weird stuff about those parts is a couple of lines in setup. I wonder if tone would be the most annoying one, just to get the 841 tone() onto timer2.... But even that is just cut and paste and find replace...