http://www.allaboutcircuits.com/projects-preview/103715

 

Hi @Raymond Genovese,
A couple quick notes that you may choose to address or not. I had a few minutes break from writing and wanted to share my thoughts. My apologies for the length, as I didn't have time to write a more concise summary. [Pascal]

1) If I interpret the datasheet you provided correctly, the addr pin controls the I2C address. If it is tied to logic high, the I2C address is 1011100 (0x5C), and logic low 0100011 (0x23). The datasheet states "ADDR, SDA, SCL is not stable if DVI 'L' term ( 1us ) is not given by systems. In this case, please connect the resisters(sic) ( approximately 100kOhm ) to ADDR without directly connecting to VCC or GND, because it is 3 state buffer for Internal testing." So this is more of a question than a correction -- but shouldn't ADDR be tied to ground rather than be allowed to float in the connection circuit?

2) Supplied Project Source Code Files, filename "BH1750.ino" lines
15:#define BYTES2READ (2);
16:#define BH1750addr (0x23);
#define is a preprocessor command that replaces all instances of the variable in the header and body of the file. When you hit the upload button, the compiler goes out and collects all dependent files and somewhere in "Wire.h" include section are some other files it needs, etc... and while it's pulling all the code in, it replaces whatever you said to replace in #define. So my comment is two-fold -- 1st) is BYTES2READ in a file your program needs?, I looked for it for a few minutes and couldn't locate it, which doesn't mean it is not there. I was just curious 2) BH1750addr (0x23); is a local constant and best practice usually has it in the current program only, unless there is a good reason not to.

3) " Looking at the program listing, notice that we first #include wire.h and use the statement wire.begin() in setup(). Then, we call the function initBH1750(), also from setup(). Finally, we read the sensor values within the loop().
To initialize the sensor in initBH1750() we use the statements:
(1) Wire.beginTransmission(BH1750addr);
(2) Wire.write(BHmodedata);
and, then (3) Wire.endTransmission()"

You spend a major part of the article referencing code that you don't present to the reader. If you're going to discuss it, you should include it using the code block in the content editor. Many readers use mobile and cannot download a zip file, and open a .ino file for cross-reference.

4) The status variable in your code seems to be recursively defined and varying in datatype. Its first defined as an integer with no assigned value, then a byte is returned to it fron the function initHB1750(). I believe it would make more sense to the reader if the status value was explicitly defined and held to a single datatype unless a conversion is necessary, which I don't believe is the case here.
19: int status;
25: status=initBH1750();
26: if (status){
27: Serial.print("Error ");
28: Serial.print(status);
29: Serial.println("Could not write BH1750 mode");
30: }
51:byte initBH1750()
52:{
57: if ( (status=Wire.endTransmission() ) ) {
59: Serial.print(status);
61: }
63: return (status);
64:}

5) "To initialize the sensor in initBH1750() we use the statements (1) Wire.beginTransmission(BH1750addr); (2) Wire.write(BHmodedata); and, then (3) Wire.endTransmission()"

You actually only call the Wire.endTransmission() inside a conditional if() statement inside the initBH1750() function. Going back to comment 4), I think the code here "works" but is not best practice. Consider explicitly starting the transmission, reading the data, and stopping the transmission. Additionally, did you mean to use the assignment operator (=) or the comparison operator (==)?

6) I believe you are shifting 8 bits of data out of existence. You define lux as being a byte (8 bits), and BHlxdata as a 2 byte array. Then shift the first 8 bits of data 8 bits to the left, where the lux variable only has 8. Essentially lux = BHLxdata[1].
35: byte lux;
38: Wire.requestFrom(BH1750addr, 2);
39: BHlxdata[0] = (byte)Wire.read();
40: BHlxdata[1] = (byte)Wire.read();
42: lux = ((BHlxdata[0] << 8) | BHlxdata[1]) / 1.2;

7) I'm now in the part where I'm looking at the provided file BH1750.c and I see some answers to questions I had earlier. The c file has some solutions for you. For example
20: uint8_t BHlxdata[2];
21: unit16_t lux;

This is a great opportunity to provide programming comments:
uint8_t BHlxdata[2]; // unsigned integer, 8 bits, 2 byte array.
21: uint16_t lux; // unsigned integer, 16 bit.

Did you write this code or get it somewhere? There is no attribution provided in the files. If it's yours, take credit for it. If it's not, give credit for it. Regardless, my personal opinion that the best place to describe what is happening in code is to comment the code. I'll also reiterate my previous comment that you should provide the code to the readers before you begin to discuss it.

8) (2) BHlxdata[0] = (byte)Wire.read(); BHlxdata[1] = (byte)Wire.read();
I'm reasonably certain this isn't the preferred method for implementing the byte conversion, which would have the command inside the parenthetical, e.g. byte(Wire.read()); Although I don't know that it's necessary in the first place. BHlxdata was created as a 2-byte array. Wire.read() is pulling in a byte. What's the purpose of the conversion? BHlxdata[1]=Wire.read(); should be sufficient.

9) First, instead of including wire.h, we will include qm_i2c.h. This header file and the associated .c file contain the source code for the I2C routines. It is a very good idea to become familiar with these two files if you want to use and understand I2C on the boards
Okay -- unless they're protected by copyright, I'd like to see a link to those files. They're not included in the .zip file you uploaded and I don't want to comb through previous articles to find them if all I'm doing is reading code.

10) The first included code segment
/* Enable I2C 0 */
clk_periph_enable(CLK_PERIPH_CLK | CLK_PERIPH_I2C_M0_REGISTER);
/* set IO pins for SDA and SCL */
qm_pmux_select(QM_PIN_ID_6, QM_PMUX_FN_2);
qm_pmux_select(QM_PIN_ID_7, QM_PMUX_FN_2);
It's an unfamiliar development board to me. I don't know if I should assume that pin 6 (from the function) is CLK or pin 7 (from the comment.) The sole comment differs from the code. Consider including some comments after the code to let the reader know what is what. I don't know how to read this without more documentation.

Raymond, that's all I have time for this evening. I hope you find the comments constructive rather than contrary. I value your long expertise and knowledge in the subject area and apologize in advance that not everything is obvious to me. Should time permit I'll go through the second half of the article.

Respectfully,
Mark

 

“and apologize in advance that not everything is obvious to me” – n/p Mark, just send me $20 to make me whole J

Thanks much for takin a look at it. I have tried to address your comments below, but I want to state up front that the article assumes some interest/experience in the D2000 board and it is part 3 and 4 of a series. I was also trying to make it meaningful for both intermediate skill level as well as those venturing beyond the Arduino.

“You actually only call the Wire.endTransmission() inside a conditional if() statement inside the initBH1750() function. Going back to comment 4), I think the code here "works" but is not best practice. Consider explicitly starting the transmission, reading the data, and stopping the transmission. Additionally, did you mean to use the assignment operator (=) or the comparison operator (==)?”

No, not at all. Wire.endTransmission() is always called – it is not “conditional”. What is conditional is printing out the error message if the transmission failed, based on the returned value of the call.

if ( (status = Wire.endTransmission() ) ) {

Serial.print("Error ");

Serial.print(status);

Serial.println("Could not write BH1750 mode");

}

Status is assigned the returned value of the call – using a comparison operator would just be wrong. The call returns a value. The variable status is assigned that return value. The variable status is evaluated, after the call, to determine if the transmission was successful or not. It’s not at all the case that the code just happens to “work”. It is intentionally written the way I wrote it and I think it is correct.

“Consider explicitly starting the transmission, reading the data, and stopping the transmission.”

Wire.endTransmission() starts and ends the “actual” transmission. Despite the nomenclature, Wire.beginTransmission() sticks the data into the buffer and never starts any transmissions. Reading the data is another call and separate from writing.

It is a little insulting to have the code criticized as “accidently working” but “fundamentally flawed” – seems like that is what you are implying. I don’t think that you understand the code. Now, it is me apologizing for being too harsh.

“4) The status variable in your code seems to be recursively defined and varying in datatype. Its first defined as an integer with no assigned value, then a byte is returned to it fron the function initHB1750(). I believe it would make more sense to the reader if the status value was explicitly defined and held to a single datatype unless a conversion is necessary, which I don't believe is the case here.
19: int status;
25: status=initBH1750();
26: if (status){
27: Serial.print("Error ");
28: Serial.print(status);
29: Serial.println("Could not write BH1750 mode");
30: }
51:byte initBH1750()
52:{
57: if ( (status=Wire.endTransmission() ) ) {
59: Serial.print(status);
61: }
63: return (status);
64:}

“

Huh? Status is declared as an int and it is declared as an int only once and its type is never re-declared (which would result in an error). We must have wildly different definitions of what recursive means because nothing like that is going on. You do have a point that the return value from initBH1750() is a byte and it is stuck in an int – mea culpa. It, of course, functions ok that way, but technically it is a bit “sloppy”, so I changed the function to return an int.

“6) I believe you are shifting 8 bits of data out of existence. You define lux as being a byte (8 bits), and BHlxdata as a 2 byte array. Then shift the first 8 bits of data 8 bits to the left, where the lux variable only has 8…. “

You are absolutely correct. While “cleaning up” the code, I decided, in a boneheaded fashion, to change the declaration of lux to a byte – which of course makes no sense. It has been corrected to an unsigned int and the .zip has been revised. Thank you for noticing and I mean that sincerely. I test all of my code files repeatedly – including that one. The problem was, and I know this is an excuse; I have all the code for the D2000 and that one bit of Arduino code – which I wrote a very long time ago. So, yes, I “cleaned” it up and tested it – in that I pulled the Arduino back out, unplugged the D2000 cables, slapped the Arduino on and ran it and, of course, it runs. I didn’t even bother to objectively look at the lux values scrolling across the screen. BTW: as I recall, you once asked in the forum how to do just exactly what that section does (or was supposed to do) – I wonder if that is why you noticed so clearly – in any event, I’m glad that you did.

“Did you write this code or get it somewhere? There is no attribution provided in the files.”

Really Mark? You think I would go take someone elses programs and stick them in an article? WoW.

“If it's yours, take credit for it. If it's not, give credit for it.”

I wrote ALL of the code for ALL of the articles I have published. These sometimes include some pretty standard stuff that has been done many times by many folks, but it is my original work – errors and all. When I get paid for an article, the article, including the code, no longer belongs to me. If someone wants to read the article and use the code, it is up to them to attribute it correctly. If they subsequently publish it without appropriate citations, AAC should do something about it – if they want. If someone wants to steal it and call it their own, I am not going to be able to stop them, or understand why they would want to do that. I do not consider myself to be some kind of master C programmer who must protect their legendary works of art. If the code is useful, I did my job. If you want to put your name in your code, I have no problem with you doing so. It is not my style in this case.

“Regardless, my personal opinion that the best place to describe what is happening in code is to comment the code. I'll also reiterate my previous comment that you should provide the code to the readers before you begin to discuss it.”

I disagree. I know that some authors include what seems to be the entire program in the article. That is not my style. I try to include code fragments and then discuss what goes on in that fragment because I feel that it is important to understand that part of the program. I don’t try to explain all the code line by line.

“You spend a major part of the article referencing code that you don't present to the reader. If you're going to discuss it, you should include it using the code block in the content editor.

Many readers use mobile and cannot download a zip file, and open a .ino file for cross-reference.”

I don’t care if people can’t access archived files on their phones – if that is the only place that they will look at the code, they are not likely to be running it any time soon.

“This is a great opportunity to provide programming comments:
uint8_t BHlxdata[2]; // unsigned integer, 8 bits, 2 byte array.
21: uint16_t lux; // unsigned integer, 16 bit.”

I’m sorry, but to me that is simply unnecessary commenting.

“8) (2) BHlxdata[0] = (byte)Wire.read(); BHlxdata[1] = (byte)Wire.read();
I'm reasonably certain this isn't the preferred method for implementing the byte conversion, which would have the command inside the parenthetical, e.g. byte(Wire.read());”

No. I am typecasting the returned value and I believe that it is done correctly. “byte(Wire.read());” would cause an error for a number of reasons.

“Although I don't know that it's necessary in the first place. BHlxdata was created as a 2-byte array. Wire.read() is pulling in a byte. What's the purpose of the conversion? BHlxdata[1]=Wire.read(); should be sufficient.”

I can’t argue strongly, but I think it is more accurate. I believe that Wire.read() returns a virtual integer (look for yourself in Wire.h): , not a byte – remember, we are talking about variable types. Functionally, the conversion will take place. It is not a big point and is probably a bit superfluous.

“9) First, instead of including wire.h, we will include qm_i2c.h. This header file and the associated .c file contain the source code for the I2C routines. It is a very good idea to become familiar with these two files if you want to use and understand I2C on the boards
Okay -- unless they're protected by copyright, I'd like to see a link to those files. They're not included in the .zip file you uploaded and I don't want to comb through previous articles to find them if all I'm doing is reading code.”

Uhhh, of course they are copyrighted – they are Intel system files for the D2000 Board. They will be on your machine when you install the System Studio. This is why I included the path of where you can find them on your machine – “You will find them among your installation directories - \IntelSWTools\ISSM_2016.1.067\firmware\bsp\1.1\drivers and IntelSWTools\ISSM_2016.1.067\firmware\bsp\1.1\drivers\include”.

Look, I don’t fault you for not automatically knowing this if you have no experience with the board and limited experience with c, but jumping to a copyright issue (again) seems a bit out of place.

“It's an unfamiliar development board to me. I don't know if I should assume that pin 6 (from the function) is CLK or pin 7 (from the comment.) The sole comment differs from the code. Consider including some comments after the code to let the reader know what is what. I don't know how to read this without more documentation.”

For the purpose of that code snippet, it does not really matter and adding a comment like “pin 7” can actually be confusing because a couple of pin name schemes have been used. What matters are that the schematic has the appropriate connections and those (sda and scl) are clearly labelled on the board. Before you can use them accurately, you need to select the correct function as the I/O pins are multiplexed – that is the important point to understand in that snippet. You can however easily look – I included a link to a very handy graphic that Intel put out (https://software.intel.com/sites/default/files/managed/57/e8/d2000_pinmap.PNG) linked right around that place in the article.

“1) If I interpret the datasheet you provided correctly, the addr pin controls the I2C address. If it is tied to logic high, the I2C address is 1011100 (0x5C), and logic low 0100011 (0x23). The datasheet states "ADDR, SDA, SCL is not stable if DVI 'L' term ( 1us ) is not given by systems. In this case, please connect the resisters(sic) ( approximately 100kOhm ) to ADDR without directly connecting to VCC or GND, because it is 3 state buffer for Internal testing." So this is more of a question than a correction -- but shouldn't ADDR be tied to ground rather than be allowed to float in the connection circuit?”

It took me a while to understand what you are talking about here – the BH1750. You are looking at the data sheet and not considering that the chip is included on a module with additional circuitry. As far as I can see, the addr pin on has a pull down resistor to ground with a connection to the module’s ADDR pin to both the chip addr and the “top” of the resistor (before the ground connection, which would be a short). You can connect the board’s ADDR pin to Vcc if you want the alternate I2C address. I didn’t go into any of that because I am using 0x23 as the address in all 3 programs and the figures have the modules ADDR pin as n/c. If I had a legitimate and linkable schematic for the board, I would have included a link to that, but I don’t and there are none at the purchase sources that I linked to.

“1st) is BYTES2READ in a file your program needs?, I looked for it for a few minutes and couldn't locate it, which doesn't mean it is not there. I was just curious 2) BH1750addr (0x23); is a local constant and best practice usually has it in the current program only, unless there is a good reason not to.”

The answer to 1 is yes, particularly for consistency with the D2000 version which is what is intended (especially after I correctly added it in place of "2" in the Arduino version [good catch]). As for 2) I don’t know what you mean by “best practice” and I don’t think the term should be used as some kind of projective test. If you can explain why an approach is flawed and suggest a revision, it is very helpful for me and the article. If you do and I can understand it, I am likely to make the revision because it makes a better article and me a better author (and programmer in this case). Saying that it is not in the “secret best practices guide book” does not do much for me especially if it is code for “I wouldn’t do it that way”.

I (and many others) typically define the device I2C address up front, where it can be easily changed if necessary. I guess that is my practice, best or otherwise. I don’t see, from what you have written, why it is wrong – although I could also see just using a global variable to define the address – I do that sometimes. Some people don’t like using #define at all. Again, I am not claiming to be a Master c programmer, but I don’t see this as a major point.



“Raymond, that's all I have time for this evening.”

Thank goodness J

“I hope you find the comments constructive rather than contrary.”

Couldn’t they be both?

Seriously, for catching that one stupid bonehead error, I am very happy. I know that you were “tapped” to take a look and I really do appreciate your efforts. If my responses are a bit harsh, I apologize.

Thanks again,

Ray