Added reset() function

[yanbec]

Related issue # and issue behavior

I didn't create a related issue, because my problem has probably it's root cause not with this library, but is in fact an electronic problem. I have one of the BME280 chips in combination with an CCS881 connected to a NodeMCU-Board. While developing a working library for the CCS chip (the existing ones just don't work for me), the BME sometimes just kind of crashed and returned one or more values as "nan". Rebooting the ESP didn't help.

Description of changes/fixes

To overcome this issue I looked up the documentation and found that the possibility of to reset the chip isn't currently implemented in this lib. So I took care of it ;)

@mention a person to review

Maybe @finitespace wants to take a look? Of course, I appreciate every comment.

Steps to test

1. Run BME until crash (yeah, that's.. not that easy if you don't have the problem.)
2. Call reset()
3. Call begin()
   (Alternative: begin(), reset(), begin(), see if it's still running)

Any outstanding TODOs or known issues

I don't know, maybe you prefer to put the necessary begin()-call into the reset()-function, I just implemented what the function name says it does. In my opinion that's a matter of taste.

Best regards,
Yannick

[finitespace]

Thank you for contributing.

A couple items:

1. Have you run and tested the code you have committed?
2. I agree with putting the begin() function inside, if it needs to be called.
3. I have a bme280 and ccs811 connected to an Arduino and haven't had any issues. You are using I2C I assume? Are there any other devices attached to the I2C bus? Have you tried calling the begin function again and that didn't work? How long does it take to replicate the crash?

[yanbec]

Thanks for your answer.

1. Yes, but it's more a "works for me" kind of test, as I only tested on the NodeMCU with ESP8266 and one BME280.
2. Feel free to change it as you wish.
3. These are the only devices on the bus, but I'm not using the "standard" NodeMCU-I2C, but SDA&SCL reversed.
   Wire.begin(D1, D2);
   Calling begin() again didn't have any effect for me.
   Btw I really appreciate this lib doesn't have a Wire.begin() call in it, as I always have to get rid of those calls manually when not using the standard pins.

Actually, I don't seem to have those problems, at the time of writing the PR it took something between 20 minutes and about 5 or 6 hours to crash.

[coelner]

I correlate this issue with this failure: esp8266/Arduino#1025. the wire library for esp32 use a fix: espressif/arduino-esp32#678
Nevertheless we need this function, but I would it not hardcode begin() into the reset() because we limit the user in his decision. We could extend it toreset(bool retriggerBegin=true)

to make the library more failsafe, we should check the Wire.requestFrom return value.

[jirkaptr]

I mean the addition of "WriteRegister(RESET_ADDR, RESET_VALUE);" into Initialize() before ReadChipID() is the best solution.
After the considered reset(), it is inevitable to call begin(). Reset BME280 can be done unconditionally within begin() - it can not hurt anything. Then there is no need for a separate reset() function.

However, there is a more serious problem with a temporary failure or sensor power drop-out, or even by replacing the BME280 sensor. Actually for example after sensor power dropout (after NAN values ended) some constant unrealistic values are read forever. These phenomena need to be detected and begin() must be called repeatedly (possibly with a hidden reset) automatically. ReadTrim() could usually be omitted.

[finitespace]

@coelner, is there ever a situation when you would not immediately call bme.begin() after calling reset? Can you give me a use case of something you might want to do in between reset and begin?

[finitespace]

@jirkaptr the problem with this is the reset requires a wait time, which limits how the user can use the sensor. Is it the libraries responsibility to detect this, or is it the users responsibility? What if the user wants to handle it a certain way, the library shouldn't dictate the recovery steps.

[coelner]

@finitespace resetting other devices without starting this one. Especially if we reset all devices because there was a hickup at the bus.

IMHO the library should be user-friendly, there-fore integrate a failover procedure. But this should be defined at the begin() call as opt-out. But I can understand the easyness of jirkaptr recommendation.
ReadTrim() is necessary if the sensor was replaced. Sensor power dropout is usually out of scope, we have to assume that this is running stable. Temporary failure could be a part, this should be fixable by checking the return value from Wire.requestFrom().

[finitespace]

@coelner, you would want to call reset on other devices before calling begin on the bme? What purpose would that serve?

[finitespace]

@jirkaptr, I didn't quite follow the second part of your post. I don't think it is within the scope of the library to detect a replacement of the sensor. If you are replacing the sensor, you should probably be resetting your controller and everything should be initialized from scratch.

Temperature failure and power dropout I can get on board with. I am not seeing these failure states in the datasheet. @coelner if we check the return from requestFrom and it returns false, you think the library should automatically reset the device?

[coelner]

@yanbec Do you have the possibilities to use a logic analyzer with the i2c bus? I'm not sure about this bug
@finitespace I can't image a real world case. Let skip that.
@finitespace That would be an easy implementation, at least to the user. But this need some more thoughts. We need to refill all registers (previous selected values for StandbyTime, Oversampling and Filter) which could take some time. Otherwise the user get unexpected values, but in this time we are blocking all other traffic on the i2c bus, because our library has got the 'token' to the bus. If we want to prevent this we need a timer or something like an interrupt driven approach which flips a flag as soon as the values get valid. It is somehow related to the topic #61. (Hopefully this is understandable)

A generic reset call within the begin() seems to be a good idea and leads to a defined starting point.

[yanbec]

Hey!
Didn't really have the time to follow up on this issue. In fact, my BME seems to be quite stable now. My trust in the values it provides is another topic.
I don't have the equipment to monitor what's happening on the bus, and as said it does not seem to happen anymore anyway.

In my opinion, a sensor library should enable the user to communicate with a device in an easy way. If a problem with the communication or the device itself occurs, this should not be masked or handled (automagically) by the library. That way, it gets way harder for the user to find the root cause of the problem. Same applies for error handling, replacing sensors or power dropouts. If you don't know about it, you can't solve it.
My proposal would be to just include the reset function and make sure it's properly documented. If somebody needs it, they can make use of it in their own way, and if it's just for testing which device/device combination/part of the code is responsible for the hickup. I hope you get my point.

By the way, thanks for the vivid discussion, I wasn't really expecting this much interest in a single line of code ;)

[jirkaptr]

Probably I'm involved by many years of work in the industrial area, with huge systems usually running continuously. The need to push RESET is there one of the most horrible things. However so strict requirements may not be applied for Arduino.

I like the saying "In simplicity is power and beauty". It is nice to have a small but widely usable SW modules. Here the matter is (only) about the library, over which has to be built application (7 layers ISO model...). The library cannot be designed to cover the needs or wishes of a wide spectrum of applications and applicators. It should only provide the tools for creating higher layers of SW. Cannot be used to interfering with outside of its own device (e.g. in a crash on the I2C bus).

Why to have or not to have a alone-standing function or automatic reset()?

• Who, or what, will call reset()? It will be a human operator, watching the screen or LED indicator? Will he do it by pressing a button or write a command? Or the application will call it on the base of failure detected according to the next point?

• Why and in what cases he/it will call reset()? No doubt, in the event that something bad happens. The library will have to detect the problem and to report to the human operator or to another part of the SW.

In any case, the library has to detect and report the problem with the sensor and it must be easy to use it. In that I agree fully with @coelner. In other words, it is necessary to generate a STATUS that shall be published. How to deal with this status? Either human or machine operator processes it, as shown above.

Error detection is already possible, and it is easily reached via the function(s) returned value. Primitive error detection plus an automatic solution represents the loop while( !bme.begin()){ Record it! + Do something! } located in the setup(). A somewhat drastic example of the status is the measured value equal to NAN. I like the idea of checking the return value of the Wire.requestFrom() or so.

@finitespace, maybe replacing of BME280 on the fly is not on the agenda, but (repeated) calls bme.begin() allows you to change the sensor because it calls ReadTrim() and sets all registers.

@finitespace, @coelner, @yanbec, automatic immediate reset is dangerous due to likely cycling while an error continues. In any case, it is necessary to provide the error report to a higher layer of the program!

Brief conclusion:

Detection of errors should be implemented in the library. How the higher SW layer (or human user) treats the detected error is its/his thing and responsibility (logger, visualized/sent message or lamp, manual or automatic attempt to recover). To call bme.begin() is usually necessary because the considered reset() is not powerful enough. If someone wishes to have special features that go beyond the usual features of the library, he/she should create them individually on his/her repository.

[finitespace]

Alright, so I think it is on the user to handle the error, but the library can detect it. In this regard it looks like the function returns NaN on read error. To remedy having to check for NaN, I am gathering that an interface change is necessary to have readData return a boolean.

Are any of the other functions used (hum, press, temp)? Should the interface be changed for them with a reference to the value and a boolean return? Or should they just be removed?

I don't see any reason for begin not to be called on reset, so I will add it to the reset function.

[finitespace]

Can you add the begin call after the delay and submit again?

[jirkaptr]

I think the most effective will be, when @yanbec makes changes in his repository. No additional forking and pull requests will be needed. (I have not write access permissions to his repository)
Reset() should return a bool, received from begin() respectively from Initialize(). (Initialize() is the only effective content of the begin()).
Begin() becomes a subset of reset().

[yanbec]

Hi!
Sorry for the delay, I now added the begin()-call to reset() and made it returning the begin's return value.

//edit: @finitespace