SpenceKonde / megaTinyCore

Arduino core for the tinyAVR 0/1/2-series - Ones's digit 2,4,5,7 (pincount, 8,14,20,24), tens digit 0, 1, or 2 (featureset), preceded by flash in kb. Library maintainers: porting help available!
Other
544 stars 141 forks source link

@MX682X - wire ISR question. #970

Closed SpenceKonde closed 11 months ago

SpenceKonde commented 1 year ago

On a few other libraries, we've Put the ISR's as well as the functions that enable them into a separate file, so that the handler code won't be included in the binary if the optional ISR's that library is concerned with, which are enabled only if the other functions in the same files as the ISR are called.

Does TWI have any interrupts that only fire during master, or only fire during slave? If so, could we move to a separate .c/cpp file the arguimerntless begin() and all master-only ISRs, and the other begin()'s and slave ISRs to a different .cpp file, that should let people who aren't using master mode to not have to keep the ISRs and vise versa. Looking through the assembly listings, and how little the code did. I'm just thinking of this because it made some major savings where we were able to pull it off on other libraries.. and it usually went in easy

MX682X commented 1 year ago

Master is not enabling the ISR, it is only polling the flags, but the Slave is using the ISRs, but I'm not sure it can be seperated due to the Class/struct/module mess I had to come up with (thus the board menu with 4 options) It's worth trying it out though.

Maybe to clarify: Yes, the library is using slave interrupts, but only if the slave begin function was called.

MX682X commented 1 year ago

What do you think about the merged TWI buffers defne/feature? It was only available through a define, so I have no idea how many people actually know about it, but as noone has complained about the RAM usage, I guess people don't need it, thus never discovered it in the first place? Basically, I'd like to deprecate it and make the library a bit better readable.

SpenceKonde commented 1 year ago

One would like then to have the slave interrupts defined in aseparate compilation unit. The problem is just figuring out the right way to make the linker only pull it in when needed. That's what I couldn't figure out that crushed my dream of a better attachInterrupt, which would, as long as the pins it was passed were constants known at compile time, not require any manual declaration of which pins to enable interrupts on nor a tools menu in order to operate without grabbing EVERY pin vecrtor. I was so fucking close, everything else works, but I couldn't figure out a way to make the reference tothe other compilation unit not get created if it could be shown not to be reachable by constant folding. A really crushing defeat. all I got was flash savings from it instead of the real prize.

Wait the merged buffers were not automatically enabled whenever they were safe to use? o_o That was not my intent. I thought we defined that whenever it was legal to do

MX682X commented 1 year ago

It was not enabled as there is a timeframe between beginTransmission() and endTransmission() where the user fills the buffer with write() that could have been overwritten by a slave interrupt

MX682X commented 1 year ago

... actually, the more I think about it, the more likely it is that it should work without issues as long as TWI_MORS is selected. I'll see what I can do while trying to make an interrupt driven master write function in a seperate file.... having the MCU do nothing for 20ms just to fill an oled screen is not fun

EDIT: I think the reason it worked with Serial was because there is only a constructor an no other function. So it won't work with Wire... Anyway, I couldn't make an Interrupt driven Wire without adding like 300 bytes of FLASH, so scrapping this idea...

SpenceKonde commented 1 year ago

Yeah, I thought the logic was that we always enabled it in TWI_MORS only, since that's the only safe mode to do it in. Otherwise, yeah the slave interrupt could fire during a transaction andscribble all over stuff. But I thought we had proved a while back that the merged buffers were safe

With the ISR's, what I was hoping was that if we exiled the begin methods that take more than one argument, and the slave ISRs, to a new file, then the slave ISR would only get pulled in if a begin method with more than one argument was called? That's not going to work (see I don;'t have a very deep understanding of what can and cant be done

SpenceKonde commented 1 year ago

I can confirm that the buffer switching is in place whenever MANDS defined. It does not check for dual mode being enabled. I reckon it should and should not do buffer switching if dual mode is allowed and needs both buffers then. It may be that the buffer sharing is something that will only be enjoyed by the tinies. But that's okay, because they have tighter constraints to deal with. .

SpenceKonde commented 11 months ago

@MX682X Can you comment on my previous message? Are we currently swapping inappropriately on DxCore?

MX682X commented 11 months ago

obviously, there are no problems with the swapping, otherwise people would have complained by now. My point was about the "TWI_MERGE_BUFFER" define. As I started with another project (that takes longer then anticipated (damn my urge to optimize display libraries)), I decided to wait with the testing, but basically, if we we have the Default Arduino mode enabled, it is theoretically possible to use it afterall and save a couple bytes flash/ram. Dual Mode just means the Slave pins are different, but the library just doesn't enable the other functionality, so it should be fine.

SpenceKonde commented 11 months ago

Uh take a look at the readme - I think I added the missing dual mode slave features >.>

MX682X commented 11 months ago

yeah I noticed, but this settings just change the behavior of the pins. And the settings are only applied, if ENABLE is set, which is done by enableDualMode() .

(I still think there is a misunderstanding however, but I can't figure out the root of it)

SpenceKonde commented 11 months ago

I think there is a misunderstanding too, but am similarly unsure.

I'm unsure of what the consequences of your proposed change are - doesn't not merging the buffers mean a significant increase in memory usage for the common case?

And what is the issue with dual mode? I thought supporting dual mode was a large part of the objective of the rewrite, but it sounds like you want to get rid of it? (that's a non-starter, it's getting used in the field - there were multiple people whose projects were on hold until the new wire because they didn't have a way to do dual mode.)

MX682X commented 11 months ago

I'm unsure of what the consequences of your proposed change are - doesn't not merging the buffers mean a significant increase in memory usage for the common case?

From what I could see, it uses more space then neccessary

And what is the issue with dual mode?

Dual Mode is the seperation of the Slave Pins from the master pins, I know the naming is a bit confusing, but that's what the datasheets are using to refer to this. I do not indend to get rid of the simultanious Master/Slave functionality.

SpenceKonde commented 11 months ago

Right, but what is the issue with dual mode, as in running master on one pair of pins and slave on another? That's what I was referring to. We have people using that feature, and one person who wanted it badly enough to use our library on MegaCoreX for a 4809. Dual mode was often requested before your version of the library was available. I'd say that in terms of things you put on a feature list, dual mode support is one of headliners. It's something that a lot of people want and which nobody else supports. Why the hell would we want to get rid of it? People use it to talk to low voltage masters and 5v slaves on mvio parts, they use it when they have parts that that don't play well, or that they don't have any addresses left for, that kind of thing. It's not a feature that's going unused.

MX682X commented 11 months ago

The Dual mode is just setting a bit in a register, nothing changes on the software side (imagine two different sub-modules with one having two 2:1 pinmultiplexers). This gives you the following options:

As in MORS the other mode is always disabled and can't be accessed, I thought about using a single Buffer for TX and RX with the TWI_MERGE_BUFFER define/code I've prepared. I tried to figure out a way how I could break the Wire library if that were the case, but I couldn't. The START and STOP conditions aswell as R/W protect the bus well enough to avoid any collision.

SpenceKonde commented 11 months ago

Wonderful!