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

Fix Wire library regression plus a little optimization #400

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

MX682X
Copy link
Contributor

@MX682X MX682X commented Feb 14, 2023

also some adjustments in the documentation

@MX682X
Copy link
Contributor Author

MX682X commented Feb 15, 2023

Since you are still working out the details on the core (I guess), there is one more thing I stumbled upon by chance:

"in r0, 0x3b" "\n\t" // RAMPZ
"push r0" "\n\t" // on the stack.

"pop r0" "\n\t"
"out 0x3b, r0" "\n\t"

You are always pushing RAMPZ, even though only a handful of parts even have this register. I suggest to use the built-in gcc define__AVR_HAVE_RAMPZ__to save some bytes on the smaller parts by checking if it is defined

@SpenceKonde
Copy link
Owner

Thanks, as you can see i was thinking only of the AVR128Dx parts when writing that library.

@SpenceKonde SpenceKonde merged commit 29c4e6e into SpenceKonde:master Feb 16, 2023
@SpenceKonde
Copy link
Owner

fixes #396 #401 #393

@SpenceKonde
Copy link
Owner

Yo we got a little problem here. There are currently differences from megaTinyCore version. I don't know which ones are the correct ones in all cases:
Wire.cpp

 *@return     size_t
 *@retval     amount of bytes copied
 */
size_t TwoWire::write(const uint8_t *data, size_t quantity) {
  twi_buffer_index_t i = 0;
  for (; i < quantity; i++) {
    if (TwoWire::write(*(data++)) == 0)
      break;   // break if buffer full
  }

  return i;
}

vs

 *@return     uint8_t
 *@retval     amount of bytes copied
 */
size_t TwoWire::write(const uint8_t *data, size_t quantity) {
  twi_buffer_index_t i = 0;
  twi_buffer_index_t qty = (quantity >= BUFFER_LENGTH) ? BUFFER_LENGTH : quantity; //Don't overfill the buffer.
  for (; i < qty; i++) {
    if (write(*(data + i)) == 0) break;   // break if buffer full
  }

  return i;
}

Not sure which direction this should be merged.

twi.c

/*@retval     error code, if enabled. Not 0 means something went wrong
 */

vs

/@retval     amount of bytes that were written. If 0, no write took place, either due
 *            to an error or because of an empty txBuffer

Which is correct? this sounds really familiar.

vs

I'm planning to add dirty_tricks.h to DxCore, and was thinking to make Wire more portable by putting

#ifndef _fastPtr_y 
(definition copied from dirty_tricks.h)
#endif             

Does that sound reasonable?

I'm guessing we want the DxC version of this one, and assume it's what fixed the bug?

    _data->_bools._toggleStreamFn = 0x00;

vs

    _data->_bools._toggleStreamFn = 0x01
;

@MX682X
Copy link
Contributor Author

MX682X commented Feb 16, 2023

about write (buf, length) - The changes that are part of this PR make the array write about 22 bytes shorter. Before that, there was an icall to write (byte). And as we check for bufferoverflow in the write(byte), there was no need for a second check.

the return value documentation was not corrected since the publishing, so it was still about the incorrect implementation. So I decided to fix it while going through the code. The Wire.cpp file is documented correctly.

and _toggleStreamFn = 0 is the fix, as explained in the discussion

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