Usart Constants and enums - mixed deprecated #259
Replies: 8 comments 2 replies
-
Is this correct:
or should it be
|
Beta Was this translation helpful? Give feedback.
-
Also doesn't this mixed enum thing cause problems all over the place, not just here? I'm sure there are tons of places where group codes are OR'ed together.... because that's how the IO headers are meant to be used, and the version of the compiler that Microchip uses doesn't deprecate that. Can you check that SERIAL_DATA_5 matter and submit a PR to fix UART_constants.h? |
Beta Was this translation helpful? Give feedback.
-
Hi, I find the mixed enums only in DxCore. Do you have more mixed enums in DxCore? I know, because of your multi controller support. :-) Microchip has created any defines. Sometimes without understandable logic. MC also does not program C++ but only C and assembler. Because of It doesn't really matter. Only the first is a byte return value. The second is an int return value. I would use the first. I'm still doing the PR ... Link |
Beta Was this translation helpful? Give feedback.
-
I think the numeric value of the CHSIZE bit is the same everywhere. (btw- the whole point of theat 0x04 is to change it to a reserved value. We later on test for that bit (and it doesn't actually get set), and only if that bit is set do we treat it as 5-bit. That way, someone who omits the character size specification for whatever reason (likely ignorance) while specifying other options gets the behavior they were expecting (8-bit characters) not 5-bit ones, which probably only 0.01% of the Arduino community ever wants to use ;-) Yeah, I've noticed that Microchip's programming style isn't very sophisticated; that might be because their toolchain sucks at C++. Personally, I try to minimize use of C++ for one (well, two) reasons: The bad reason is that I'm not very good at it. The good reason, is that classes completely confound the optimizer, as do many other C++ features. Unreachable code ends up in binaries if it's a class method. Constants are not folded. LTO stands by and watches as incredibly inefficient code is produced which I'd expect to see optimized away to nothing.. I routinely see code in class methods where the comments claim that things are going to be optimized, you look at the assembly listing, and no optimization occurred. I have at times been extremely tempted to use some regexes on the io headers, to turn the enums they supply into #defines The regexes to do it wouldn't be hard.... It's more a matter of feeling that the I/O headers are sacred. If you wouldn't mind, can you check some of the examples for Event and Logic libraries? I know both use enumerated types, and they mat have similar issues. |
Beta Was this translation helpful? Give feedback.
-
Hi, the event examples do not show any problems. The Logic examples show problems. Here in the header defined defines are used directly which are deprecated. For the USART, you have added an intermediate layer and created your own definitions. I don't know yet what I would do here. Single register assignments or an intermediate layer. I have to think about it. Something else caught my eye. Your enums contain errors. I use a AVR128DB64. Logic:
Modulate:
C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Modulate\Modulate.ino: In function 'void setup()':
Modulate:33:23: error: 'tcb1' is not a member of 'in'; did you mean 'tcb'?
33 | Logic0.input1 = in::tcb1; // Use TCB1 WO as input 1
| ^~~~
| tcb
Bibliothek Logic in Version 1.1.2 im Ordner: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic wird verwendet
exit status 1
'tcb1' is not a member of 'in'; did you mean 'tcb'?
Oscillate:
C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino: In function 'void demo9a()':
Oscillate:467:25: error: 'tca0_cnta' is not a member of 'user'; did you mean 'tca0_cnt_a'?
467 | Event1.set_user(user::tca0_cnta); // Connect Event1 (carrying Logic0 output) to TCA0 event a
| ^~~~~~~~~
| tca0_cnt_a
Bibliothek Logic in Version 1.1.2 im Ordner: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic wird verwendet
Bibliothek Event in Version 1.2.0 im Ordner: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Event wird verwendet
exit status 1
'tca0_cnta' is not a member of 'user'; did you mean 'tca0_cnt_a'? |
Beta Was this translation helpful? Give feedback.
-
Hello, what is the error message with the new UART_constant.h? With which controller? About the Logic examples Modulate and Oscillate. Modulate: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Modulate\Modulate.ino: In function 'void setup()':
Modulate:33:23: error: 'tcb1' is not a member of 'in'; did you mean 'tcb'?
33 | Logic0.input1 = in::tcb1; // Use TCB1 WO as input 1
| ^~~~
| tcb
Bibliothek Logic in Version 1.1.2 im Ordner: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic wird verwendet
exit status 1
'tcb1' is not a member of 'in'; did you mean 'tcb'? Oscillate: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino: In function 'void demo9a()':
Oscillate:467:25: error: 'tca0_cnta' is not a member of 'user'; did you mean 'tca0_cnt_a'?
467 | Event1.set_user(user::tca0_cnta); // Connect Event1 (carrying Logic0 output) to TCA0 event a
| ^~~~~~~~~
| tca0_cnt_a
Bibliothek Logic in Version 1.1.2 im Ordner: C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic wird verwendet
Bibliothek Event in Version 1.2.0 im Ordner: C:\Arduin IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Event wird verwendet
exit status 1
'tca0_cnta' is not a member of 'user'; did you mean 'tca0_cnt_a'? About the enums in the Logic examples. C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino:474:48: warning: bitwise operation between different enumeration types 'TCA_SPLIT_CMD_enum' and 'TCA_SPLIT_CMDEN_enum' is deprecated [-Wdeprecated-enum-enum-conversion]
474 | TCA0.SPLIT.CTRLESET = TCA_SPLIT_CMD_RESET_gc | TCA_SPLIT_CMDEN_BOTH_gc;
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino: In function 'void demo9d()':
C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino:620:37: warning: bitwise operation between different enumeration types 'TCD_CNTPRES_enum' and 'TCD_SYNCPRES_enum' is deprecated [-Wdeprecated-enum-enum-conversion]
620 | TCD0.CTRLA = (TCD_CNTPRES_DIV4_gc | TCD_SYNCPRES_DIV8_gc | TCD_CLKSEL_OSCHF_gc);
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino: In function 'void demo10()':
C:\Arduino IDE Portable\AVR128DB\arduino-1.8.19\portable\packages\DxCore\hardware\megaavr\1.4.7\libraries\Logic\examples\Oscillate\Oscillate.ino:675:37: warning: bitwise operation between different enumeration types 'TCD_CNTPRES_enum' and 'TCD_SYNCPRES_enum' is deprecated [-Wdeprecated-enum-enum-conversion]
675 | TCD0.CTRLA = (TCD_CNTPRES_DIV1_gc | TCD_SYNCPRES_DIV1_gc | TCD_CLKSEL_PLL_gc);
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ Either you do on user level as I did in UART_constant.h. TCA0.SPLIT.CTRLESET = static_cast<uint8_t>(TCA_SPLIT_CMD_RESET_gc) | static_cast<uint8_t>(TCA_SPLIT_CMDEN_BOTH_gc); Looks unfamiliar stupid to many people, though. Correct according to C++ standard. TCA0.SPLIT.CTRLESET = TCA_SPLIT_CMD_RESET_gc;
TCA0.SPLIT.CTRLESET |= TCA_SPLIT_CMDEN_BOTH_gc; Or you create an additional layer with own name like UART_constant.h where you cast hidden. Whether this really makes sense I don't know. According to my opinion you can't wrap the CCL in a lib anyway. There are too many possible combinations. In the examples you can see that despite the lib still much is done with direct register assignments. I generally program the CCL manually. You always have to read the manual. But first we fix the problem with the UART_constant.h. How can I help? Did you change something in your toolchain? Used new controller header files? Microchip likes to change the bit names. I have often noticed this with the AVR DB. I hope you can understand the translation. |
Beta Was this translation helpful? Give feedback.
-
Sorry for being unclear, usart_constants works. The unexpected compile error I referred to was the issue you found in the examples. And I got the examples compiling now (this was a problem that I didn't know existed because they're not in CI - misspelled a constant in oscillate, and used the tcb1 input in modulate (tcbs asCCL inputs aren't done like that on full size parts, only tinies - Logic was first ported to tinies, then from there to Dx, Event went the other direction - I can't blame MCUDude for not wanting to deal with the tinyAVR 0/1-series event system. It's a total shitshow, the designers clearly didn't know what they wanted until they'd gone too far down the wrong path to fix it) And yeah, I usually use the CCL directly - but note that in the examples, the stuff being configured with direct register writes is the other peripherals being used as event sources/users! Every combination of options on the CCL is supported through the library. The CCL itself isn't terribly complicated. That it can connect to every other part of the chip and do all manner of -- So they don't like us ORing enums.... but that's how microchip intends us to use them. Microchip really should have just used defines for the group codes instead of fucking around with enums like that (then we could test for them in preprocessor statements too!), but as we know, they're not thinking about C++ - and we probably don't want them to, god knows what they'd screw up if they tried. I don't even know if splitting writes to CTRLESET will even work correctly; it might be order dependent - CTRLE is a really wacky register. TCAs and TCDs each have at least one weirdo register that has a set and clr register, and can autoclear itself when you issue a command - I don't fully understand those registers, the only thing I do with that register on a TCA is the hard reset command). It's clear that they had planned to allow for some commands to go to one channel at a time in split mode but then discovered it didn't work right and fixed it by removing all the options except NONE and BOTH. If the deprecated warnings are coming from code that is in the example, I think they stay, as the "correct" way is not deprecated on the version of C++ that we're compiling for and makes the code less readable, compromising it's usefulness as an example (and those examples are ugly already), and someone who has changed to C++ 20 doesn't need to use the examples as written. If there are places where we do that in the library, I think using static casts is the appropriate fix. If you didn't see, they released a first look at the headers for EA-series, so the EA is coming. Big changes to EVSYS, but the ADC looks like it's a direct copy of the 2-series tiny. And as with the 2-series,. they left in a chunk of header for internal use that's definitely not going to be there at release. I don't like the idea of putting an extra layer between the user and the headers - we had to keep that for USART for compatibility with Arduino (and we also had to pack the parameters into as few bits as possible while providing as many options as possible) -- I hate the way Microchip has been changing bitfield names on us. Every time they make a breaking change, I have to add code to core_devices.h to make sure that either name works! I don't know why they are changing those names either. FREQSEL to FRQSEL? CRYSTAL to XTAL? |
Beta Was this translation helpful? Give feedback.
-
Hello, okay, all is well. My text should not be a reproach to you or MCUdude. You are doing a great job. Full respect. As always, it's all about very small subtleties. 😄 I also think you better leave it with the CCL as it is. Because of me nothing had to change. The enums I get myself under control. 😉 Because of the question about the SET and CLR registers. I think I can help there. You can do it all yourself in the VPORT registers. Then you have to take care of all bits yourself like before. So read and clear them with & ~ etc. The SET/CLR registers do this work for you. It is the same with TCA.CTRLCLR/CTRLSET. Realization: In the SET/CRL registers you only have to use OR to manipulate single bits. You only think positive about the bit you want to change. The SET or CLR register then determines whether this bit is set or cleared. You get used to it very fast. The bit name renaming in the header files also annoys me. I wanted to ask Microchip already once what that should. Whether they can agree finally once. But I never did. I would have a crazy idea because of the enums. You write your own headerfile. This time sorted by registers. All bit definitions of a register belong to an enum. Makes also logical sense. Makes a lot of work and must not contain a single error. It's just a crazy idea. 😉 Translated with www.DeepL.com/Translator (free version) |
Beta Was this translation helpful? Give feedback.
-
Hi,
I will address the topic of UART constant again separately.
I often work with current avr-gcc toolchains. Many syntax is set to deprecated. For example that you should not mix different enums. Therefore I have to edit the UART_constant.h. It currently looks like this for me. This should not have a negative impact on other users. But it helps me a lot and others who use newer toolchains.
Example:
My modified UART_constants.zip
Now I wanted to ask if you would take over this?
Beta Was this translation helpful? Give feedback.
All reactions