Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding PTC! (Beta) #1019

Merged
merged 3 commits into from
Oct 14, 2023
Merged

Adding PTC! (Beta) #1019

merged 3 commits into from
Oct 14, 2023

Conversation

MX682X
Copy link
Contributor

@MX682X MX682X commented Oct 10, 2023

I really hope I haven't effed up anything...
Wanted to finish on Sunday, but came up with the idea to make more then one callback function...

I can see how the next few months might end up with a lot of support requests and expansion of the docs and compile-time checks... (thinks of Wire)

@SpenceKonde
Copy link
Owner

SpenceKonde commented Oct 11, 2023

Woah! Nice. And perfect timing for a beta too, since we just did a release and it will give the people a chance to poke at the PTC.

This is not related to anything other than 1-series tiny, right? 0-series and 2-series didn't get it, and DA got a different version?

We now have a naming hazard: I don't want another Flash.h (both DxCore and mTC come with Flash.h. These were written simultaneously by two people with vastly different approaches to programming who were unaware of the other's work. Thus the API is completely different on DxCore for writing the flash than mTC. and the EA's will require a solution more like the mTC one. So I've kind of painted myself into the corner.

I'm still stuck in that corner, and sooner or later, I'm going to have to do something about this. I don't see any alternative to bashing a hole in the wall or running across the wet paint [ie, breaking changes to all who use those]

(Once a library is released, changing the name of the library punishes everyone who used the library before you renamed it, they upgrade the core and their sketches would stop compiling. Besides I need to retain the moral high ground when Microchip renames things in the io headers :-p )

So what that means is that we need to look into the future, what are the unknowns?
We do not know if Microchip will release any more parts with a PTC
We do not know if nobody, you, or someone else will make a DA PTC library.

  • If microchip never releases anything with a new version of the PTC
    • If nobody writes a DA PTC library
      • The name doesn't matter, PTC is fine
    • If someone eventually does write a DA PTC library
      • I would ideally want that library in DxCore too
      • If such a library would have an essentially identical API, or if it API was a superset of the tinyAVR one (something which you can only predict if you're the one writing it, or you know a reason it would be very different), then the ideal result would be one library with a big chunk of #ifdefs to choose the implementation for the part, so PTC is a reasonable name for the library (though one could argue for names that were not a "jargon" abbreviation)
      • If such a library would likely need a significantly different API that would mean a different library, which would have to have a different name if the API is different (that's a mistake I don't intend to repeat). In this case, PTC is not a good name, because we could end up with another PTC library for DxCore coming along with a completely different API, and then it's name would have to be changed to prevent confusion from arising from two different same name libraries (remember that significant portions of the documents are shared). In that case, I don't like the idea of awarding one of the parts an unqualified name and adding a qualifier for one of them (if there end up being libraries with different APIs, I'd rather not have them named either the same as eachother, or as .h and .h or .h. That sort of naming implies that the name with the qualifier added on is a derivitive or adapted version of the other, and that the unqualified library serves a larger product line and larger range of parts. But I think the tiny1 and the DA are on about the same tier (it;'s not like a tiny0, which everyone, rightfully, treats like a dead skunk. They were the budget model, and they hacked off half the peripherals, while shaving off only pennies from the price. In contrast the DB looks to be maintaing supremacy among DxC users. And as I expected, the 1-series remains popular - many people have no need for a more accurate ADC, but want the 7th and 8th PWM pin, or more than one comparator or (I hope, occasionally) the second ADC). In this case ("Same/compatible API unlikely if a DA library were to be written") I'd be thinking about them having qualifiers on the name for this and any DA library that might appear.
      • In any event, I would opine that PTC is not a great name for a library, simply because it's too close to and looks too much like the name Microchip would assign to a peripheral, rather than the name of a library to use one. (The only case where we violate this "don't use the same name as a peripheral" guideline is for SPI.h (obv not optional, for compatibility - otherwise, we have Logic, not CCL, Event not EVSYS, Comparator, not AC, Flash not NVMCTRL, Wire instead of TWI. Serial instead of USART. etc.
        (Did you know that mTC and DxC can both actually use the S in USART through HardwareSerial now? Some special considerations needed - if you're using clocked USART, you're doing something weird, you're almost certainly a reasonably competent programmer, it's not hard, just slightly ugly because I didn't want to burden the rest of the userbase. who wasn't and never would use it (especially since this group outnumbers people who would use this feature by a large ratio, quite possibly an infinite one. But we can do it now. I don't know WTF we'd use it for, but we can. It's wacky, master or slave is determined by the XCK pin's DIR bit, and maybe you're thinking it sounds a lot like the MSPI mode, and not too different from SPI? Well yes, except that MSPI mode and SPI mode only turn on the clock when they're sending data. But in USART, both sides are allowed to send data at any time, so there has to be a clock on XCK whenever communication might happen. Oh - and even with that, USRT can't recognize when the start of a byte is naturally (SPI can recognize this, because data appears along with clock pulses, and USRT can't do that because if you turn off the clock the slave can't talk... So the data is sent with framing bits! It's a strange one. No idea what, if anything, uses it.

Re: the above issue with the Flash.h;s
So the DxC Flash.h API can't be implemented for other parts, because you don't have to write a whole page at a time on Dx (among other things, this let's you attenuate erase intensity, and I understand that it's erasure that's the thing you count for lifespan, which is a huge issue on Dx because we lost a 0 in a clarification on the endurance). And I couldn't implement the API of Flash.h either except by putting a wrapper around my code; that library relies on the bootloader having more code in it, which is not an option of the Dx-series. Tiny's bootloaders are a lot smaller. Some of these parts are ah, a wee bit tight on space. For example, the largest AVR Dx bootloader looks to have uh, 6 bytes of flash available in the bootloader section (and when I went and looked at thefile, I saw only 5 bytes free in the hex. And it's much less ignorable of an issue on modern AVRs than classics because of how little EEPROM we get.
(t161x/162x Dx, 42x 82x) FLASHSIZE/8 >= RAMSIZE >= FLASHSIZE/32 [example: t167, m2560). with FLASHSIZE/16 = RAMSIZE being most common on classics, with all moderns at least FLASHSIZE./16. But EEPROM has changed dramatically, Classic Rs would typically have FLASHSIZE/8 >=EESIZE >=FLASHSIZE/32. On an AVR32DA it's FLASH/64...
A table makes the contrast even more stark (these are Flash divided by EEP/Userrow/RAM, so large numbers mean less of that memory per unit flash. UROW included because it's another form of non-volatile storage.

Part Ram Ratio EE ratio UROW ratio
Most Classic Tiny 16 16 n/a
167 32 32 n/a
87 16 16 n/a
328p 16 32 n/a
tiny0/tiny1- 16 32-128 64-512
tiny1 16k 8 64 512
tiny1 32k 16 128 512
tiny2 8 32, 64, 64 (128, 256, or 512)
tiny2 32k 10.67 128 1024
AVR32DAB 8 64 1024
AVR64DAB 8 128 2048
AVR128DAB 8 256 4096
AVR64DD 8 256 2048
AVR32DD 8 128 1024
AVR16DD 8 64 512
AVR64EA 10.67 128 1024
AVR32EA 8 64 512
AVR16EA 8 32 256

So obviously, there is a lot more demand for writing to flash here, when people used to flash/32 of NVM they can write, suddenly move to a chip with perhaps 1/256th as much EEPROM as flash. So I didn't feel like I should just ignore the issue, and I wrote my library for Dx. Around the same time MCUdude wrote one for megacorex which works on tinyavr too (they use the same nvmctrl). Shortly after I added that to the core I got a PR for the other library on mTC (where I wasn't pkanning to do a flash writing library because I couldn't understand how the optiboot flash mechanism worked - though on the Dx, the structure naturally lent itself to being separated from needing optiboot; the other one is fully reliant on it.

The style of the code and the types of things we do anddo not put guardrails around is dramatically different, and the API's are totally differnet (neither of us knew the other one was doing it, then we had two libraries which were specific to a subset of hardware and matched in name, Then a part was released in the DxC purview... but which needs tiny-like flash handling.

Yeah I'm in a bit of a tight spot on that. But first I need to make it upload to an EA with a programmer that I own,

@MX682X
Copy link
Contributor Author

MX682X commented Oct 11, 2023

  1. I'm planning, if I get the time to do it anyway, to use #include guards to supply the functionality to 1-Series and DA. I've started doing it a bit in the code already, but realized that I have to basically disassmble the DA qTouch from like the beginning, due to the different Register set (see files). And my time budget significantly sunk.

  2. Flash. From the 30 mins I had so far to look through the docs and code, I'd say: Use Flash.h for page access and EEPROM.h for Byte access. As in: The first 512 Addresses will be to EEPROM, everything above is considered a write to Flash with a -512 byte offset. And use Flash.h on the Dx to emulate page Writes/Reads

@SpenceKonde
Copy link
Owner

Ugh. I had a nice reply typed out here and lost it.

The reason I mentioned name (and Flash as a result) was that at this juncture, we must pick a name for the library and we must pick the correct name, because once released, the name shall not change, come hell or high water [for moderate values of either] to avoid another Flash.h situation.

Re: Flash - I think you missed the issue - both parts have EEPROM.h and USERSIG.h for writing to those spaces (no, they can't use the same library - tinyAVR and mega0 could have, and in fact did. Dropping the USERROW out of EEPROM.h I think came with mTC v2.0, as it was a breaking change, but needed to present the same behavior on DxC and mTC, and do so coherently, and the mechanics of the userrow on the two parts meant that none of the code that mattered was portable. The new USERSIG.h used the same file (with large parts #ifdefed out ) since the Dx userrow has flash-like (10,000 1000) rewrite endurance and you can only erase the whole thing. Thus, unlike any other memory area on the Dx, the userrow needs to have a software buffer that they can make changes to before sending the order to write it to the userrow (it's actually a bit more complicated - write functions will write to the hardware until they are asked to write a value with a 1 bit where a 0 bit was. That requires an erase, and so we discontinue opportunistic immediate userrow writes and buffer that and furture bytes until the user flushes the buffer to the hardware, and all the write functions pass a value back to the caller indicating if the changes were written to the chip, or if they could not be written to the chip automatically because an erase is needed, and provides a function to do that, as well as to refresh the buffer from the hardware, clear the buffer, and erase the userrow directly, On mTC the USERROW functions always report success, and the functions related writing the buffer always report that it was successfully written, and never do anything else (they're just there for compatibility) except for the erase one, which (i hope and belive) does erase it on mTC as well).

So that's how I like libraries to be if they need significantly different implementations on Dx and tiny (and most of them do) - same API and same file and same name.

The remaining memory is the flash - and that's problematic:

  1. tinyAVR must have the code to both write data into the nvmctrl buffer (by storing to the location in flash where the write is targeted) and writing to NVMCTRL.CTRLA located in the bootloader section.
  2. the optiboot flasher library is the low level interface to that, i found it incomprehensible at the time.
  3. Dx-series, however, only needs one operation to be in the boot section: The st, sts, or spm which actually causes the write/erase to happen.
  4. Dx-series bootloader has almost no slack space for write to flash from app code, so the hook had to be supertiny. The obvious way to do it was if we called, from assembly, the address of an SPM instruction, followed by return. only 2 words of the remaining space in the bootloader section - (placed at the end, at a known offset, by placing a numerical constant of the appropriate value there), leaving 3 words of slack for future changes required in the bootloader, if any should be required. I sure hope not though, because 3 words isn't much space... After that is exhausted the payback of development time for flash gets dramatically worse, as you dredge the assembly listing looking for bad renderings and rewriting the relevant code in asm to fix it. There is one known instance of pathological register manipulation that could be entirely elided if rewritten in assembly (it's pushing and popping a register (and those are the only references to it, and it doesn't get carried around either with an adjacent value. Why are we wasting a word and a clock to push a register that contains no meaningful valuie (uninitialized) then a word and 2 clocks to pop it?! (I suspect the answer is that the compiler fails to produce rational code for very small programs, not recognizing that it doesn't need to save and restore a register that never holds a meaningful value anywhere else. That there are only 3 words of flash left after optimization thorough enough that I know off the top of my head that there are 3 words left and where to look for 2 more should tell you something about the level of optimization effort that has been directed towards optiboot_dx and thoroughly sink any hope one might have that the identical interface could be provided through any means short of emulating it, which violates the "do not hide the light of the parts under a barrel" principle of this core (as that would drop the coolest feature of the Dx NVMCTRL, the word writes.
  5. So we are in a can't-win situation:
    a, We cannot make the tinyAVR Flash.h act like the DxCore one, the hardware is not capable of it.
    b. We cannot make the Dx bootloader act like the tinyAVR one, there is no space
    c. So making the DxCore library act like the tinyAVR one would involve an emulation layer.
    d. And the end result, by definition would be worse than what we have now, in terms of capability, because it would haveto hide the features of the Dx.
    e. Thus we cannot make the DxCore library emulate the tinyAVR one without replacing a library with a less capable one.
    f. Above and beyond those concerns, there is the fact that I can't stand the tinyAVR Flash.h API - if you look at the two libraries, and realize how none of the parts has any . It offends all of my programming instincts. But am I going to reimplement tiny write to flash? Hell no. TinyAVR got Flash.h after I categorically said that I would never write one and did not feel that it was warranted, and then unexpectedly, just as I released DxCore with Flash.h of my design. a new Flash.h is introduced on MCX and PR'ed to mTC and I failed to have the good sense at the time to realize that something needed to be done, and it had to be done then, because we don't rename libraries and we cant make the libraries act like each other.
  6. Thus we are left with two incompatible libraries of the same name on the two cores, and no route towards replacing them that does not require compromising principles.

This wasn't meant to be a request for assistance, just as a way of underscoring the importance of figuring out the right name. I'm merging this in it's current form (though it would be pulled before any release, and then put back in if a name wasn't settled on.

If there are plans to use the same api or a superset of the api for the DA, then we absolutely want the same name on both cores, with no qualifier; if a future part has a PTC (I am uncertain we will ever see this again) and it's drastically different and cannot be accommodated using the same API, so be it; if someone then wrote a library for it, that would get a qualifier after the name or a different name altogether if it's API was different.

The question then becomes what to call it. I don't really like calling it PTC - precedent is ProperCaps not TLAs (TLA = "Three Letter Acronym"), and particularly not a an initialism or abbreviation that is defined in the headers, because there is strong precedent for anything of the form ., being indeed supplied by that library and providing it's functionality, while . is something supplied by the I/O headers, so library names should not be the standard initialism of that or any other peripheral, or any other sequence of characters likely to be used in Microchip's I/O headers
. We have Logic not CCL, Comparator not AC, Event not EVSYS and so on. And Serial instead of USART. The only exception is the SPI library, which we didn't have a choice on the name of - and indeed it actually did cause us problems).

@SpenceKonde SpenceKonde merged commit d3a4314 into SpenceKonde:master Oct 14, 2023
35 of 88 checks passed
@MX682X
Copy link
Contributor Author

MX682X commented Oct 14, 2023

Dx-series bootloader has almost no slack space

Some ideas:
A. consider putting "MYUART" into the Y register (the fast_ptr hack) . This needs experimenting ( I don't wanna set up a build envoirement) however this might break the "fill buffer with data from uart" part in an unpredicatable way as it is using the frame pointer for increments (and it adds an epilogue of
mov r28, r24
ldi r29, 0x00 ; 0
mov r29, r28
eor r28, r28
)
B. consider changing the order of the attributes of:
void writebuffer(int8_t memtype, addr16_t mybuff, addr16_t address, pagelen_t len)
to:
void writebuffer(pagelen_t len, int8_t memtype, addr16_t mybuff, addr16_t address)
This might help as the register allocation order is 24, 25, 18, 19 etc. By having len imediately in a subw register might help gcc with allocating the variables to the right registers more efficiently.

(I don't have the listing with source to analyse the code further)

The Reason why I suggested to use the EEPROM library for the byte wise access of the Dx-Flash is that it is similar enough to the EEPROM mode of operation. And having a compatibility layer towards the mTC wouldn't matter as much due to the bigger Flash/RAM size of the Dx.

About the naming: Maybe "Touch" would work? but it is not really unique. Another possibility would be to go back to "ptc_touch" or similar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants