Open SpenceKonde opened 2 years ago
SpenceKonde
commented
2 years ago Oh, and in the examples where it takes serial input, where millis isn't disabled, I added a 30s timeout when it's waiting for a CR or LF to mark the end of a message. So after 30 seconds or so someone using that awful serial monitor the IDE comes with will start to figure out why it's not sending what they entered.
MX682X
commented
2 years ago What is with the strange commenting formats in Wire.cpp?
I have actually no idea, I've just kept what was already there.
Why do the examples put a delay(100); into an otherwise empty loop?
Maybe that's some artifact from the beginning? I'd say it is not really needed.
On a side note: I just noticed that I had started changing the example master_read, but forgot to finish. Did it now. Sorry for that.
SpenceKonde
commented
2 years ago Thanks. slave_write was also missing a description, but I've fixed that already
Okay wacky formatting goes, lol
Is the /* (with the extra ) required for some automatic documentation thing or more crap to remove to normalize the formatting?
Another question -
The slave request handler doesn't take any arguments. How does it know how much data to write? What happens if the slave is expecting the master to read more data that it does? Say the master does a sequence like: send commands that would suggest it would ask for 2 byte response request 1 byte. send command that would suggest it would ask for 1 byte response request 1 byte - what does it see now, the byte it's expecting, or the second byte that the previous onRequest ISR put there?
Finally, sometime between now and when these releases go to board manager... one of us really ought to make an example that shows an implementation of the classic "register" model of most commercial I2C devices, which is basically universal everywhere except on I2C slaves written in Arduino. As in:
slave device docs say
0x00: major version
0x01: minor version
0x02: patch version
0x03: status: bit 0 = idle (0)/active(1), bits 3-1:error(0 = no error, otherwise an error code)
0x04: control: bit 0 = disable(0)/enable, bit3-1:number of pin to monitor
0x05: data: 8-bit value, voltage on selected pin.
master to slave, write: 0x03
slave records 0x03 as address.
master to slave read 1.
slave sends master the contents of it's status variable.
master to slave write 0x04, 0x02
slave acks and sets control variable. When it's back in loop, it now sees that it's enabled, and it's pin is set to 1. It starts calling analogRead(1), scaling to 8 bits and storing the result in it's "data register"
master to slave write 0x05
master to slave read 1
slave sends the value in the data register, which is now the voltage on pin 1 as read by (analogRead(1)>>2)What happens if the slave is expecting the master to read more data that it does?
if master read < slave write: - the slave resets the buffer position when it receives a correct address. if master read > slave write - I'm not sure what happens, but TWI can't handle this case. The TWI actually has to keep the CLK line released, otherwise it would block indefinitely. I think it sends 0xFF, basically never pulls the SDA pin down when there is no data.
implementation of the classic "register" model
I could try to write a code that gives the ability to "directly" access some of the registers of a tiny, like sigrow and adc, when do you want it?
SpenceKonde
commented
2 years ago Let's not get hung up on the word register - I mean it in terms of what the slave shows to the master. Sometimes these will correspond directly to a hardware register on the slave - but usually not.
There's almost a de facto protocol on top of I2C, where the first byte sets the "address pointer" (which points to one of a number of "registers" controlling the device and storing results), and the next byte(s) written if any will write to that "register" and the pointer usually automatically increments (occasoinally it doesn't). So the "registers" are just consecutive locations in a section of memory, when it gets a write, it sets it's pointer, and on each byte after that, decides whether to allow the master to write there (and how much), while a write followed by a read(s) reads starting at the location given by that initial write.
The purpose would be served by something as simple as a volatile uint8_t array (or to get fancy, a struct, or maybe a union of a byte array with a struct or something, so the I2C interrupts see it as a continuous block of bytes, while the rest of the application refers to fields in it like separate variables) - which could all fit into a 32-byte I2C buffer (request handler could just stuff the whole thing into the buffer with something like for(uint8_t i; i<length; i++) Wire.write(i2cregs[i+i2c_ptr]); where length <= 32) and the receive handler would set the i2c pointer to the first byte and if there was more than one, check each one against some criteria.
(eg, "There are 8 registers, registers 0-3 are the configuration registers that the master can write, registers 4-7 are data which are read-only, pointer auto increments and wraps around to 0 after they read the last one" and And in loop say, every second it could could print the configuration registers and then get 4 bytes from random() or something - the point is not what it does, but how it's structured, so it acts like a normal, civilized I2C device.
I realize now that Wire can't do this, because you never know how much the master read, and hence can't adjust the pointer.if you want it to increment rather than resetting to where they last wrote it. (I don't think I'd ever thought deeply enough about I2C as implemented on a slave that I knew there was an issue here). But it's the same way on the official library....
What prompted me to ask the first question was that I was thinking about implementing that, and then realized that there didn't seem to be a way to find out how much the master read, so how could you implement anything other than some adhoc halfassed thing that uses the I2C bus as if it were "half-duplex serial with a clock".
I had always thought "Geez, why is every Arduino I2C slave a shoddy haphazard mess? Haven't they ever used a real I2C device?!" Now I know: The Arduino team didn't provide the tools to do anything else. Considering their track record (they're practically the murphy's law of API design), I probably should have considered that the Wire API they gave us might be the problem before....
UGH!
Couldn't one make a uint8_t that gets incremented every time the master reads a byte, and something like getBytesSent() to read + reset it, or getBytesSent(bool preserve) to read and optionally reset it (IMO default should be yes reset, not resetting it is a weird thing to want to do - like if you wanted to see if they'd read all the data in your data acquisition code before replacing it or something) ? Then someone could implement the register model by, at the start of the receive and request handlers, calling getBytesSent(), and adding that to the value of the pointer so any previous reads are accounted for? that sounds relatively easy to implement and low cost in terms of flash, assuming I'm not overlooking something....
I'm going to check in code to megaTinyCore soon so you can see what I horrible things I've done to your code over there, and DxCore shortly after that.
SpenceKonde
commented
2 years ago Code is in megaTinyCore, documentation is still in progress.
SpenceKonde
commented
2 years ago ~some stuff about a buffer sizing problem~ Yeaaaarrgghhhh that would be the Serial buffer reflexing on me....
SpenceKonde
commented
2 years ago First up, writing the documentation I came upon two particular questions: When would someone want to call flush()? Is my interpretation that it's for kicking the master when it's gotten confused by junk occurring on the bus, recognizing that the buffers will be dumped and not sent (this is in contrast to Serial.flush() which empties the transmit buffer in a very specific way: by waiting until it is sent.
I remember several times being asked to implement a Wire.flush() (before I had studied the implementation of Wire()) that worked like that for the slave, so you could call it and it would wait until it "was done sending" (which I now realize should be translated to "has received the stop bit for the most recent read request"), because they wanted to be able to put the slave to sleep or reset the slave without a half-completed transfer.... Do you know if the two lowest bits of the slave status register stay unchanged until another address match or stop bit comes in? If so, is checking that bit 0 is 0 (stop bit was last received) would seem to be the way to implement some sort of test for whether the the Wire bus is busy - unless the things that clear APIF also clear AP and DIR from TWIn.SSTATUS, in which case it would be a lot harder... if a slave can just test whether testing AP !== 0 outside of the ISR, even a wrapper isn't provided, I'd like to document it...
And what is writeRead() in Wire.h? Is it supposed to be there?
Another thought - you said it resets the slave transmit buffer when it gets a new request from the master; what's the corner case requires the slave to have separate TX and RX buffers then? When were were initially talking about the merged buffers I'd thought about the two buffers that each role has being combined with the opposite role. I think we established that that can't work. But I see that what is implemented is sharing of the TX and RX buffers.
That looks to be far safer to me.... What I can't tell is whether it's "safeer" (but not totally safe), "safe" (completely), or "unsure if safe".
I think I get it for master - in Arduino-land it's legal to call readFrom() between beginTransmission() and endTransmission(), so merging buffers is a no-go
But on slave side... you say that either a read or a write addressed to the slave clears the slave transmit buffer (which I think is reasonable - not arguing)? And a slave won't write it's rx buffer while receiving until it knows that it is addressed.... So the only time it will write the rx buffer is when it has already determined that it will empty the tx buffer? At that point anything that would cause it to start transmitting again would also mean that a stop condition would be received (and onReceive called), and THEN a start condition followed by it's address and a read bit. Which would call the onRequest handler to set up the tx buffer, and Wire.read() and Wire.write() in master&slave mode clearly don't work outside the handler - that may not be true for master|slave - but if they're supposed to work on master|slave, then the buffers could be left as is there, because memory pressure is less acute there. memory pressure is the tightest for master&slave anyway. Am I missing something here? I2C is a goddamned complicated protocol compared to the usual fare of Serial, SPI, and bitbanged '2812.
SpenceKonde
commented
2 years ago Oh, and new docs are in - but I am not confident about what I wrote on Wire.flush and need to generally clean the docs up, but it's now documented, and I actually found numbers for folks about pullup sizeslatest version of library is checked into DxCore and megaTinyCore (and passing the CI)
MX682X
commented
2 years ago When would someone want to call flush()?
Good question, I can imagine two possibilities: Restarting the master by sw (I tried to use the flush bit in the registers, the module ended up in a hanged state), but in the end this would just call Wire.end() and Wire.begin() with some registered saved temporarily in a local variable. Possibilty 2: wait for the slave TX buffer to be completely read out, but I would not recomend it, beause it will need a timout and that takes space. After all, it is not guaranteed that the
Do you know if the two lowest bits of the slave status register stay unchanged
the DIR bit only changes when receiving a matching address, together wih the APIF bit. The AP bit changes together with the APIF bit aswell, but every time an Address or Stop is received. They can't be cleared by any other user interactions (except reset TWI) So it probably would work.
And what is writeRead() in Wire.h? Is it supposed to be there?
Not any more, no. I had the hope, that a single write/read function, that would be called instead of beginTransmission, endTransmission and readFrom, would end up being a bit smaller, but it wasn't... I probably should have proofread the code before saying it is done.... I was just thinking about wether or not to add functionality)
what's the corner case requires the slave to have separate TX and RX buffers then?
After thinking about it and verifying the official Arduino implementation actually resets the buffers aswell, there is indeed no need to have two buffers for the slave. Afterall, as you basically pointed out, all relevant write/reads to the slave buffers happen between START and STOP. And it is only relevant in maste&slave, since on master|slave we have to expect the user to use the master. Good riddance that the code for the slave buffer merge is basically already written....
And about the register example:
I was thinking about a struct {uint8_t* REGSTER, uint8_t r_w_flags, uint8_t protection}; and an array that consists of those structs. When the user supplies an address, it is the number for the array. The code checks if the user is allowed to access that register by the r_w_flags, and if they are not, responses with 0xFF or something like that. To authorise themselves, they might append a byte to the address that has to correspond to the protection variable.
SpenceKonde
commented
2 years ago I think beginTransmission and end transmission should stay as is. We can get away with tweaking the periphery, but not the bread and butter functionality.
You wouldn't begive some of the crap I;'ve checked in. I had a whole screen long doc where my hands were off target the WHOLE TIME. 2 versions later I opened it up, realized it was utterly incomprehensible and pulled it.
I have sentences that change structure several times in one sentence..... I do ALL sorts of stuff like that myself. Right now I'm looking at your serial stuff, because I'd love to get a checkmark in that box - but more importantly, I am getting a million requests for RS485 functionality, people complaining about it people asking how to do it - I gotta implement that shit ASAP; which means the new serial stuff should be in. (Who the fuck uses RS485?! Apparently like everyone and their mom does., plus some household pets.
What do you think about my getBytes Read() idea to get the number of bytes the master read from slave buffer Is there any weird complication to implementing that? My thinking is a uint8_t that gets incremented, located same place as the getIncomingAddress data is stored and the function will always reset the value ti 0 when read unless the user passes true to it to tell it to preserve instead of resetting?
I don't think the slave buffer transmitting wait-until-finished needs a timeout. Because it's only valid until the bus gets a stop condition - unless your concern is that the bus could be hung? (since any attempt to revive the read after that would result in onRequest being called again)?
MX682X
commented
2 years ago What do you think about my getBytes Read()
I suggest to get a function similar to Available, but on the serial tx buffer, so basically bytesLeftToRead.
I gotta implement that shit ASAP
Well, at least I added some uncomented code as an idea for the pins in the UartClass::_set_pins function for the XDIR/XCLK pins...
Because it's only valid until the bus gets a stop
good point, I was thinking that the master could read less then expected and the buffer being not empty would leave it in a hanged state
Why do the examples put a delay(100); into an otherwise empty loop? Is there a reason? I've seen that idiom in other arduino code around the internet, but I've never known why they did this. It seems unnecessary.
I've added WIRE_ALT_ADDRESS() and WIRE_ADDRESS_MASK() macros to improve readability of three argument begin() and cleaned up the buffer size comments.
I'm adapting the new examples to have a MySerial defined at the top for portability. And use 115200 baud (I am trying to train the world to stop defaulting 9600 baud on modern AVRs (in fact, less than a factor of 4 above the absolute minimum modern AVRs can do at > 40 MHz!) , because a surprisingly high number of forum questions are from people who default 9600 baud, their sketch fills up the serial buffer in moments, and then they wonder why it's running so slowly and ask for help on the forums. Though I'm trying to do it by example, not by lecturing like I do some issues.
I'm expanding the introduction to examples.
I'm making an address mask and multi address write (from your examples), to demo the masked address, and to provide a master to match the dual address slave sketch without a DA/DB-series part with 32+ pins (right now on DX-core that's onlythe28-pin parts, but every future part that will go into DxCore announced so far only has one TWI.
README.md will be adapted. Will be checking it into the repos shortly. Will add more comments as they come up.