Skip to content

Commit

Permalink
Update twi.c
Browse files Browse the repository at this point in the history
  • Loading branch information
@SpenceKonde
SpenceKonde authored Jan 17, 2021
1 parent 205d8b2 commit 5146822
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions megaavr/libraries/Wire/src/utility/twi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,12 @@ void TWI_MasterSetBaud(uint32_t frequency) {
t_rise = 1000;
}

uint32_t baud = ((F_CPU / 1000 / freq_khz) - (((F_CPU * t_rise) / 1000) / 1000) / 1000 - 10) / 2;
TWI0.MBAUD = (uint8_t)baud;

// uint32_t baud = ((F_CPU / 1000 / freq_khz) - (((F_CPU * t_rise) / 1000) / 1000) / 1000 - 10) / 2;
// TWI0.MBAUD = (uint8_t)baud;
// Prevent an integer overflow that can break the baud rate!
// Arduino megaAVR #90.
uint32_t baud = (F_CPU / frequency - F_CPU / 1000 / 1000 * t_rise / 1000 - 10) / 2;
TWI0.MBAUD = (uint8_t)baud;
}

/*! \brief TWI write transaction.
Expand Down

5 comments on commit 5146822

@MCUdude
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be right. freq_khz is still there, but not used for anything. Have a look in the official twi.c.

Why is the "new" implementation introduced in ArduinoCore-megaAVR #90 better than the implementation you're attempting to replace here?

@SpenceKonde
Copy link
Owner Author

Choose a reason for hiding this comment

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

(commas for human readability)
F_CPU = 16,000,000
t_rise = 1,000
(F_CPU * t_rise) - 16,000,000,000
This is 0x03 B9AC A000.
truncate to fit 4 bytes
0xB9AC A000

Not sure if it's significant in terms of it's impact (haven't gone any further than what I wrote above - which satisfied me that there was a problem being fixed here)

@SpenceKonde
Copy link
Owner Author

Choose a reason for hiding this comment

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

Huh, but yeah, you're right, the new formula is ALSO wrong - at least, assuming what the original formula presumably intended was correct...

@MCUdude
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant was that in order for the function to be correct, you'll either have to replace frequency with freq_khz, everywhere (and adjust the formula) or vice versa. Right now it's a mix of the "old" and "new" implementation

@SpenceKonde
Copy link
Owner Author

Choose a reason for hiding this comment

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

OH! Thanks... I hadn't realized that there had been another change in between the two which renamed a variable. Goddamnit...

Please sign in to comment.