Skip to content

Overload beginTransmission and requestFrom to support using an external buffer #289

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

Conversation

sandeepmistry
Copy link
Contributor

Replacement for #269, with updated API's based on the discussion with @lupalby. Wire. requestFrom(...) and Wire.beginTransmission(...) are overloaded to support external buffers.

The external buffer for TX can optionally be partially or fully pre-populated with data. The existing Stream and Print API's can continue to be used to populate the external buffer.

@ArduinoBot
Copy link

✅ Build completed.

⬇️ Build URL: http://downloads.arduino.cc/PR/samd/package_samd-b183_index.json

ℹ️ To test this build:

  1. Open the Preferences of the Arduino IDE.
  2. Add the Build URL above in the Additional Boards Manager URLs field, and click OK.
  3. Open the Boards Manager (menu Tools->Board->Board Manager...)
  4. Install Arduino SAMD core - Pull Request Overload beginTransmission and requestFrom to support using an external buffer #289
  5. Select one of the boards under SAMD Pull Request Overload beginTransmission and requestFrom to support using an external buffer #289 in Tools->Board menu
  6. Compile/Upload as usual

Copy link

@lupalby lupalby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that overall it looks great!
Just a couple of details could be improved. Also, your bracing style does not match the rest of the file (although it was not very consistent to begin with)

}
else
{
if ( externalTxBufferLength == externalTxBufferQuantity )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use >= for robustness

}

void TwoWire::beginTransmission(uint8_t address) {
void TwoWire::beginTransmission(uint8_t address, uint8_t buffer[], size_t length, size_t quantity) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name of the arguments in beginTransmission is confusing. How about bufferLength and usedBytes?

@PaulStoffregen
Copy link

PaulStoffregen commented Dec 22, 2017

Some time ago we all discussed an "addMemory" API to be added to HardwareSerial, Wire, and other libs. I believe @facchinm made an initial implementation, but it never left a branch on his fork, maybe due to lack of agreement over the API details?

Is that idea gone now? Is passing pointers really going to become the Arduino API?

@sandeepmistry
Copy link
Contributor Author

Closing in favour of #298 for now.

After internal discussions we have decided not to add additional API's to the Wire class. Instead PR #298 increases the Wire buffer size of 256 for both TX and RX.

boseji pushed a commit to go-ut/combined-ArduinoCore-samd that referenced this pull request May 12, 2021
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.

4 participants