bme280 forced mode about periph

[maruel]

I totally agree this is desired but I couldn't work on it on the short term as I had more pressing issues. My feeling for an optimal solution is:

• Refactor devices.Environmental to have a function to SenseNow(env *Environment) now vs SenseEvery(chan<-Environment, d time.Duration) to sense continuously at a specified frequency. Not sure about the names, how to handle do this (blocking? channel?)
• Implement this in bme280. Stop exporting bme280.Standby and instead chose the closest lower value and when rate is above 1s, the driver use forced mode behind the scene.
• In the calculation of the correct Stanby value, take in account the actual measurement overhead Tmeasure,max is described in section 9.1 of the datasheet. A time.Sleep() of slightly less than Tmeasure,typ should be done then a continuous read should be done until the value is available.

[quinte17]

I have to ask another general question.
I do not see why its defined Sense(env *Environment) error instead of Sense() Environment, error?
The same applies to the planned SenseEvery() method. Why not use something like the time package:
func Tick(d Duration) <-chan Time so this would be: SenseEvery(d Duration) <-chan Environment

[tve]

In general allocating a channel and returning it is not great because you then have to decide in the library how much buffering you allocate for the channel. I don't think this is a big issue here, but it's annoying to have to make that choice when writing the code.

[maruel]

For Sense(), I felt it made the code easier to code as you can do:

var e devices.Environment
for (
  if err := d.Sense(&e); err != nil {
    return err
  }
  // use e
}

and save on memory but I agree it's not really much a big deal either way as returning a struct is merely copying it on the stack, it is not using the heap. TL;DR; I don't have a strong opinion there, the API just eventually need to be consistent.

About the channel as input or as output, Thorsten resumed my thought. Some users may want a buffered channel, some may not want that. The issue is that sending data over a channel is unpredictable, it can block. This is tricky for the sender. In some way, you may see this as I am advocating to create the channel internally but I am not, the data should also include timing information, likely adding time.Time to the message sent over the channel.

Another way is to add instead WaitForNextSensing() which is the pattern I used for gpio with PinIn.WaitForEdge(). Not sure it's actually great but it leaves more options to the caller.

I'm open with experimentation for trial and errors, so we can see what works out best in practice. Just be warned up front I may change my mind frequently through the process. :D

[tve]

FWIW I really like the WaitXx() pattern for the gpio edges, it puts control over goroutines into the caller.

[quinte17]

why not use consts for register adresses? that would also make some comments useless.

        // ctrl_hum
        0xF2, byte(opts.Humidity),
        // config
        0xF5, byte(opts.Standby)<<5 | byte(opts.Filter)<<2,
        // ctrl_meas
        0xF4, byte(opts.Temperature)<<5 | byte(opts.Pressure)<<2 | byte(normal),

would become

        regCtrlHum, byte(opts.Humidity),
        regConfig, byte(opts.Standby)<<5 | byte(opts.Filter)<<2,
        regCtrlMeas, byte(opts.Temperature)<<5 | byte(opts.Pressure)<<2 | byte(normal),

[maruel]

I don't like const that are used exactly once. It actually makes the code less readable, the reader has to jump through the file to get the actual value.

[quinte17]

the proper ide has the tooltip which tells the value. so there is no point to jump anywhere in the code.
and if one address ever has to change (because version 2 of the hardware comes out and the driver could be nearly the same) then you would only have to change the value of the const.
and you gain the benifit of auto-completion on symbols. the magic number does not have it.

also many other hardware projects do it like that. through all other languages.
one of the most known projects also doing this is the linux kernel ;)

for example the i2c-driver for at91 of the linux kernel has many definitions like that:

#define AT91_TWI_IER            0x0024  /* Interrupt Enable Register */
#define AT91_TWI_IDR            0x0028  /* Interrupt Disable Register */
#define AT91_TWI_IMR            0x002c  /* Interrupt Mask Register */
#define AT91_TWI_RHR            0x0030  /* Receive Holding Register */
#define AT91_TWI_THR            0x0034  /* Transmit Holding Register */

#define AT91_TWI_ACR            0x0040  /* Alternative Command Register */
#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
#define AT91_TWI_ACR_DIR        BIT(8)

#define AT91_TWI_FMR            0x0050  /* FIFO Mode Register */
#define AT91_TWI_FMR_TXRDYM(mode)       (((mode) & 0x3) << 0)
#define AT91_TWI_FMR_TXRDYM_MASK        (0x3 << 0)
#define AT91_TWI_FMR_RXRDYM(mode)       (((mode) & 0x3) << 4)
#define AT91_TWI_FMR_RXRDYM_MASK        (0x3 << 4)

also the "exactly once" is no argument. the config register is used in more than two places.
making exceptions just for one register doesnt make sense. so we either stay or convert all.

[quinte17]

so just wanted to start a little bit and play in that direction. if you want you can look at
github.com/quinte17/pio/tree/feature/bme280-forced-mode
another question that came up:
when using forced mode I cannot write only the forced bits without reading it in the first place.
we have 2 options

• use read modify write to trigger forced mode
• remember a copy of registers or a copy of options in the device struct

whats your opinion?

[maruel]

Interesting I don't have access to the repository (i.e. ACL doesn't propagate when forking), so you'll have to grant me access to it.

[maruel]

Fixed with af3fbce.