Refactoring I2C code

This forum is dedicated to software development related to MultiWii.
It is not the right place to submit a setup problem.
Software download
Post Reply
Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Refactoring I2C code

Post by Tommie »

While fixing the broken GPS code, I made some changes to the i2c code. You can find them here: https://github.com/wertarbyte/multiwii- ... c_refactor

I changed the following things and would like to see those changes integrated into the official Multiwii tree:

- unify i2c_readNak and i2c_readAck into one function
- add i2c_read_reg_to_buf to read multiple bytes in a single call
- add swap_endianness function to change to endianness of a transferred buffer
- changes i2c addresses of peripherals to 7bit notation
You might ask why? Nearly every I2C library utlizes this notation, and most data sheets also specify the 7 bit notation of the device address. I also changed the code "ADDRESS + 1" to "ADDRESS<<1 | 1", since adding 1 will not yield a correct result if the LSB is alread set for some reason.

I also removed a few occurances of undefined behaviour:

E.g. i2c_readReg16 does the following:

Code: Select all

return (i2c_readAck() <<8) | i2c_readNak();

C(++) however does not define the orderin which both subexpressions are evaluated; depending on the compiler, optimizer settings and lunar phase i2c_readNak might be called before i2c_readAck, trashing the transfer (https://en.wikipedia.org/wiki/Sequence_point).

Thanks for reading, please feel free to leave feedback :-)

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: Refactoring I2C code

Post by kos »

- +1 for 7bit -> move all sensors address to sensors.h
- move i2c function to i2c.c and use nice wrapper ( one read == one call , one write == one call)

porting mwi to linux :)

using http://www.lm-sensors.org/browser/i2c-t ... /i2c-dev.h


Code: Select all

 
//BMA180
int res = i2c_smbus_write_byte_data(i2cBus, 0x0D, 1<<4); // register: ctrl_reg0, : set bit ee_w to 1 to enable writing


Code: Select all

static int i2cBus;

void i2c_init(void) {
        int i2cBusId = I2C_BUS_ID;
        char i2cBusName[20];

        i2cBus = open_i2c_dev(i2cBusId, i2cBusName, sizeof(i2cBusName), 0);
        if (i2cBus < 0 ){
          DEBUG("Can not open i2c bus");
          exit(1);
        }
}

void i2c_getSixRawADC(uint8_t add, uint8_t reg) {
  set_slave_addr(i2cBus, add, 0))
  int i;
  for(i = 0; i < 6; i++)
  rawADC[i] = i2c_smbus_read_byte_data(i2cBus, reg+ (i));

}
Last edited by kos on Sat Apr 14, 2012 11:47 am, edited 2 times in total.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

kos wrote:- +1 for 7bit -> move all sensors address to sensors.h
- move i2c function to i2c.c and use nice wrapper ( one read == one call , one write == one call)

Yes, I thought about that, but one step at a time :-)

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: Refactoring I2C code

Post by Alexinparis »

Tommie wrote:While fixing the broken GPS code, I made some changes to the i2c code. You can find them here: https://github.com/wertarbyte/multiwii- ... c_refactor

I changed the following things and would like to see those changes integrated into the official Multiwii tree:

- unify i2c_readNak and i2c_readAck into one function
- add i2c_read_reg_to_buf to read multiple bytes in a single call
- add swap_endianness function to change to endianness of a transferred buffer
- changes i2c addresses of peripherals to 7bit notation
You might ask why? Nearly every I2C library utlizes this notation, and most data sheets also specify the 7 bit notation of the device address. I also changed the code "ADDRESS + 1" to "ADDRESS<<1 | 1", since adding 1 will not yield a correct result if the LSB is alread set for some reason.

I also removed a few occurances of undefined behaviour:

E.g. i2c_readReg16 does the following:

Code: Select all

return (i2c_readAck() <<8) | i2c_readNak();

C(++) however does not define the orderin which both subexpressions are evaluated; depending on the compiler, optimizer settings and lunar phase i2c_readNak might be called before i2c_readAck, trashing the transfer (https://en.wikipedia.org/wiki/Sequence_point).

Thanks for reading, please feel free to leave feedback :-)


Hi,

You might ask why? Nearly every I2C library utlizes this notation, and most data sheets also specify the 7 bit notation of the device address. I also changed the code "ADDRESS + 1" to "ADDRESS<<1 | 1", since adding 1 will not yield a correct result if the LSB is already set for some reason.

A 8 bit I2C address should always have its LBS to 0, if not it's a bug and we should not implement code to care about this.
The problem with 7 bit notation: there is always a confusion to check if it's a 7 bit or 8 bit one. I saw to many examples of this confusion around the web.
With 8 bit notation, there is no possible confusion as nothing is related to a 7 bit address in the code


Code: Select all

return (i2c_readAck() <<8) | i2c_readNak();

right, it's in the very new code form guru_florida to read sonar. we should probably remove this ambiguity.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Alexinparis wrote:A 8 bit I2C address should always have its LBS to 0, if not it's a bug and we should not implement code to care about this.

And that's the point: There is no such thing as an i2C address with 8 bits. The I²C reference design by Philips specifies that the address space consists of 7 bit. When addressing a device, these 7 bits are transferred over the wire - appended with an additional bit to indicate whether a read or write request is about to occur.
(See http://www.nxp.com/acrobat_download/lit ... 340011.pdf, page 15ff)

Specifying this 8 bit address in source code introduces a kind ambiguity that will lead to bugs, for it mixes the device identifier with an access identifier. Using the addition operator to switch between read/write modes introduces even more opportunities for bugs, since (address + 1) might not only modify the intended operation, but also change the device identifier; of course, this issue can be mediated by using binary operators instead (one of my changes does this for nearly every ocurance).
The problem with 7 bit notation: there is always a confusion to check if it's a 7 bit or 8 bit one. I saw to many examples of this confusion around the web.

Actually, there is no confusion. According to the referene documentation, an I²C address has 7 bit, period. Storing this address in a constant and generating the wire format from it by bit shifting and appending the desired transfer direction is IMO the cleanest method to prevent coding mistakes. Es you already said, the LSB should never be set in an 8-Bit-Address, but when specifying 7 bit addresses, this mistake isn't even possible.

Code: Select all

return (i2c_readAck() <<8) | i2c_readNak();

right, it's in the very new code form guru_florida to read sonar. we should probably remove this ambiguity.

As I said, feel free to use my patches. At the moment, I am hunting down a few other occurances like this :-)

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: Refactoring I2C code

Post by kos »

could the i2c refactoring done by Tommie be merge in before the release of 2.1 ?

Federico
Posts: 64
Joined: Thu Apr 05, 2012 12:32 am
Location: Italy
Contact:

Re: Refactoring I2C code

Post by Federico »

The point to 7bit addresses looks valid. Every datasheet referes to 7 and not to 8 bit...

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

If you want to merge my stuff, please do so before it gets completely out of sync. I am trying to keep up with development, but merging does not get easier over time.

If I can provide any assistance, please just ask.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Any news about this issue? I just synced my code up to the latest svn version, please consider merging it.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: Refactoring I2C code

Post by Alexinparis »

Hi,

changes i2c addresses of peripherals to 7bit notation

Ok, if everyone thinks it's vital to change this, we can integrate this change.
And you're right, it's the I2C norm.

For other changes, your code is more generic and it's a good thing.
But it is bigger than the previous one...
make it smaller or equal and I will adopt it ;)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Alexinparis wrote:For other changes, your code is more generic and it's a good thing.
But it is bigger than the previous one...
make it smaller or equal and I will adopt it ;)

It is more or less equal, probably even shorter considering the added functionality:
https://github.com/wertarbyte/multiwii- ... les_bucket
"Showing 5 changed files with 147 additions and 134 deletions. "

So it's 13 new lines for doing transfers of multiple bytes, swapping the endianness of buffers and making the transfer of complex data structures more readable. But I don't consider the line count important.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: Refactoring I2C code

Post by Alexinparis »

I'm not speaking about the source code, but about the compiled code.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

I havent measured the difference, but I think it is neglectable. Including all my patches, my firmware file is about 24kiB, so it is far from filling the ATMega328, and the battle for being fitting into the ATMega 168 is lost anyway.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

I might add one thing: The TinyGPS interface, which is actively used by a few people here for GPS and sonar data, depends on these new functions. It's also probable that porting more and more portitions of the old code to the new function calls will yield additional memory savings.

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: Refactoring I2C code

Post by timecop »

ok, I see the benefit of using 7bit addresses everywhere, but where i DON'T see the benefit is doing the 8bit shift as function arguments...
i.e. i2c_rep_start(LCD_ETPP_ADDRESS<<1);
why not just i2c_rep_start(LCD_ETPP_ADDRESS); and do the shift once inside the function?
Is there anything that does NOT use 7bit address when doing a repeatstart?

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

What if you want to supply the read address when doing rep_start? The low level functions (i2c_rep_start) take the final address as an argument, while i2c_read/i2c_write/i2c_read_to_buf just take the 7 bit address, since they can determine the trasaction direction for themselves.

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: Refactoring I2C code

Post by timecop »

Seems overcomplicated.
After all, a generic I2C transaction can be described with a single function

i2c_transaction(uint8_t address, uint8_t *txbuffer, uint8_t txsize, uint8_t *rxbuffer, uint8_t rxsize);

1st byte of txbuffer would be subaddress (since all it is is a write following i2c address), then (some devices) want more than one byte written (example with 16bit subaddresses or somethign), then repeat start, then read x bytes into rxbuffer.
I don't see the need to overcomplicate stuff with over9k different ways.

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: Refactoring I2C code

Post by kos »

dongs , this wont change the size ..


Alexinparis wrote:I'm not speaking about the source code, but about the compiled code.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

I just checked the size difference, we are talking about roughly 80 bytes here. If MWC would be running on a ATTiny13 I could see the point in hunting down these memory pages, but since there is more than enough program space available (or is anyone really filling the flash memory already?), I somehow fail to see the point in it. Adapting more old code to the new convinience function will probably even safe more of it. So please consider merging the code.

copterrichie
Posts: 2261
Joined: Sat Feb 19, 2011 8:30 pm

Re: Refactoring I2C code

Post by copterrichie »

Well, the Pro-Mini/ATMega328 is already having memory shortage issues.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

The Promini is using the ATMega168 - ok, that might be a problem depending on the features enabled.

But IMO there are much bigger memory hogs hidden in plain sight.
I just encountered this one in Output.ino:

Code: Select all

     /* true cubic function; when divided by vbat_max=126 (12.6V) for 3 cell battery this gives maximum value of ~ 500 */
static uint16_t amperes[64] =   {   0,  2,  6, 15, 30, 52, 82,123,
                                     175,240,320,415,528,659,811,984,
                                     1181,1402,1648,1923,2226,2559,2924,3322,
                                     3755,4224,4730,5276,5861,6489,7160,7875,
                                     8637 ,9446 ,10304,11213,12173,13187,14256,15381,
                                     16564,17805,19108,20472,21900,23392,24951,26578,
                                     28274,30041,31879,33792,35779,37843,39984,42205,
                                     44507,46890,49358,51910,54549,57276,60093,63000};

So we are storing 128 byte - both in Flash as well as in RAM - but what for? What is the source of these numbers?

User avatar
Hamburger
Posts: 2562
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: Refactoring I2C code

Post by Hamburger »

Tommie wrote:I just checked the size difference, we are talking about roughly 80 bytes here. If MWC would be running on a ATTiny13 I could see the point in hunting down these memory pages, but since there is more than enough program space available (or is anyone really filling the flash memory already?), I somehow fail to see the point in it. Adapting more old code to the new convinience function will probably even safe more of it. So please consider merging the code.

on 328 with a 10DOF sensor board and LCD/OLED you run into problems (has been like that for quite some time already)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Strange, I am using an 328p with ACC, MAG, Baro, GPS and Sonar as well as some custom modifications and I am still only using around 24kiB of flash memory.

User avatar
Hamburger
Posts: 2562
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: Refactoring I2C code

Post by Hamburger »

That is with lcd / oled?

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Hamburger wrote:That is with lcd / oled?

Without, but with custom LED flashing code, automatic (sonar engaged) landing lights, TinyGPS and the Datenschlag framework to receive encoded data frames from my custom TX, yielding 4 AUX channels from a single RC channel.

User avatar
Hamburger
Posts: 2562
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: Refactoring I2C code

Post by Hamburger »

with lcd including lcd.aux I had to introduce some #undef for LCD code fragments to get down to a working binary size.
Just to clarify, I currently am neither pro nor against your refactoring i2c code.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

I did not claim that, but it is kind of frustrating to invest time into a change, build additional changes (TinyGPS etc.) upon these refactorings - and then watch the patchset rotting away in the sun because it does not get merged in while the underlying codebase starts moving again. IMO MultiWii should adopt some kind of patch management. Although I prefer git and github, this is possible with SVN and Google Code as well. Posting changes to a forum thread sure isn't the proper way to drive and encourage development.

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: Refactoring I2C code

Post by timecop »

I agree, this code should be added asap.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: Refactoring I2C code

Post by Alexinparis »

ok men, I will add all the changes if there is a plebiscite for it.
~80 bytes are not so critical.

Tommie, you can take this example to see how critical the size could be:
Try to compile a quadX config + QUADRINO_ZOOM_MS board + SERIAL_GPS on a 328p.
I'm sure you can help my to grab some bytes here and here.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Code: Select all

stefan@exa:~/git/multiwii-firmware [sizetest…] $ avr-size build/MultiWii.hex 
   text      data       bss       dec       hex   filename
      0     25558         0     25558      63d6   build/MultiWii.hex
stefan@exa:~/git/multiwii-firmware [sizetest…] $

Looks OK to me?

copterrichie
Posts: 2261
Joined: Sat Feb 19, 2011 8:30 pm

Re: Refactoring I2C code

Post by copterrichie »

What would happen if we abandon the bootloader and use a programmer to write the hex file to the chip similar to how the KK board does it.

Katch
Posts: 280
Joined: Thu Aug 04, 2011 1:44 pm

Re: Refactoring I2C code

Post by Katch »

copterrichie wrote:What would happen if we abandon the bootloader and use a programmer to write the hex file to the chip similar to how the KK board does it.


everyone would get really mad at having to retro fit an ISP header to boards that don't support it...

User avatar
Hamburger
Posts: 2562
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: Refactoring I2C code

Post by Hamburger »

we lose plenty of users. Running arduino IDE with editing config.h and triggering upload is complicated enough as it is.

Besides, it only helps avoid the inevitable so long - 328p is at its limits.

copterrichie
Posts: 2261
Joined: Sat Feb 19, 2011 8:30 pm

Re: Refactoring I2C code

Post by copterrichie »

Not really in my opinion because the IDE can be configured to talk to a programmer the same as it talks to the USB port.

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: Refactoring I2C code

Post by timecop »

Hamburger wrote:Besides, it only helps avoid the inevitable so long - 328p is at its limits.

Not according to the biggest troll in multiwii history:

nutjob wrote:As far as mega chips and arm chips - its all hype and theory we found from flight tests comparisons - dream up the most power cpu and tell people they need it - wrong - actually the 328 memory is not even close to been limited yet - in real flying - in real world flight tests - the 328 CPU does the job perfectly - if you design it correctly - and place the sensors correctly on a daughter board with the right kind of vib isolation - and our GPS works fine - we already tested it. A Paris 328 flying 6 engines and a gimbal is rock solid. Your welcome to buy cheapo boards - or brag about a mega chip blah blah - but in the spirit of RCGroups - you wont find me trolling thru your product threads and trying to run them down with causal comments. PARIS is rock solid - it's a fact - we are here to stay -


Has this guy heard of angular momentum? Being in the center of the board or on one of the arms makes zero difference to the sensors. O shit, better tell the heli guys to stop putting the gyro on the tail - better move it into the center of the frame! Or those dumb commercial airplane (not RC) designers who have the gyros in the nose. They don't know anything, their shit will never fly as smooth as a PARIS!

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: Refactoring I2C code

Post by kos »

please stop being harsh to anyone that do no fit your view.. your are not relevant by your code nor your intervention, so try to play to nice and be cool.

thanks in advance

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: Refactoring I2C code

Post by Tommie »

Thanks for merging :-)
I already started on porting the TinyGPS to the new GPS codebase, should be ready soon :-D

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: Refactoring I2C code

Post by timecop »

kos wrote:please stop being harsh to anyone that do no fit your view.. your are not relevant by your code nor your intervention, so try to play to nice and be cool.

thanks in advance


One thing is when someone is stupid and unaware of it, but this guy is clearly smart *AND* misleading his users.
That's the problem, nothing to do with code / etc.

Post Reply