i2c-device library feedback

[SpaceShawn]

Looks like a good start! This is really going to help simplify the jobs and make execution safer and more predictable. A few notes:

While compiling I'm getting a warning that should be easy to clear: src/i2c-device.cpp:53:50: warning: ‘file_handler’ is used uninitialized in this function [-Wuninitialized] sprintf(file_handler,"/dev/rtc%d",bus_number);

For the read function, ask @shawn97 (shawn@spaceconcordia.ca) and @shayden-- (ty@spaceconcordia.ca) for input on this to meet the needs of the jobs. Maybe we overload the function to allow other buffer sizes, or just take the max buffer size required by our sensors and let them handle the processing.

Please change use snprintf instead of sprintf for safety, we added this a few months ago to our coding conventions: http://libslack.org/manpages/snprintf.3.html

The logging and/or output should be handled by the application using the library, not the library itself, so your I2C functions could instead pass the error_message pointer as a parameter, or just use defined error_ids (you could add them in SpaceDecl.h).

Lastly, would like to see some tests as always! I know it's a simple library now but units tests are always part of our process. You can use the utls/AllTests.cpp or add a new test class.

Thanks Harley

[harley1011]

I did hesitate about doing the logging in the actual library itself, hence the comments. But I think the logging should be done in the library. Why should the logging/output be handled by the application using the library? To me it makes more sense to do it in the library itself. We don't have to add an extra line of code every time we use one of the functions and if we want to change something then we only have to change it in one place.

All the application needs to know is whether it was a success or a failure.

[SpaceShawn]

In most cases our logging needs to happen in the application because we need the context of the process that is calling the library, not the other way around.

For example, the read-pwr-ina219 job will use your library. In its log entries, it records read-pwr-ina219 as the process in the log entry. This will add the error to the read-pwr-ina219 log file. If something goes wrong with the job because of the library, or any other reason, we can easily scan the job logs for the source of a problem, because we will know which job failed because we are missing that job's data.

If the logging instead happens in the library, a log file will be created for the library itself and we will have no direct context about where it was executed from. Just a bunch of errors in different files that we will have to reconstruct manually (possible, but undesirable).

Since ultimately it is the job that will fail, the job of logging belongs to the application under execution, not its dependent libraries. Extra lines of code are a worthy sacrifice for clarity.

So just return the error id/status, and optionally pass the error_message as parameter.

Thanks Harley

PS - as an undocumented convention we don't like to commit commented (dead) code where possible, so just remove the Shakespeare stuff :)

Shawn Bulger

ConSat-1 Space Concordia http://www.spaceconcordia.ca

H 1029-7

1455 Rue De Maisonneuve West Montréal, Québec

H3G 1M8(514) 848-2424 ext. 2738

On Mon, Nov 3, 2014 at 9:57 PM, Harley McPhee notifications@github.com wrote:

I did hesitate about doing the logging in the actual library itself, hence the comments. But I think the logging should be done in the library. Why should the logging/output be handled by the application using the library? To me it makes more sense to do it in the library itself. We don't have to add an extra line of code every time we use one of the functions and if we want to change something then we only have to change it in one place.

All the application needs to know is whether it was a success or a failure.

— Reply to this email directly or view it on GitHub https://github.com/spaceconcordia/space-lib/issues/9#issuecomment-61586716 .