I understand you copied the structure for this driver from one of the existing drivers, but this modifiable params struct is what I would call an antipattern in RIOT. A better solution is to change this to const srf04_params_t * to let the parameters reside in ROM.

According to this page, this is a recommended pattern. But your concern sounds valid. We have the same question in #7067. @haukepetersen what's your opinion ?

I suggest moving the time, distance, and state to the srf04_params_t struct and keep only the hardware configuration parameters in srf04_params_t

Get rid of the gpio arguments here and place the configuration in srf04_params.h

Wrong docs

Missing docs

Missing docs

Does it always return this value? Is there any reason to have a return value then?

Wrong docs

I would slightly update the name of the error enums by introducing ERR: SRF04_ERR_GPIO, etc

According to this page, this is a recommended pattern. But your concern sounds valid. We have the same question in #7067. @haukepetersen what's your opinion ?

Why do you need distance in the initialization parameters ? Same question for time and I also saw below that state is also added in the parameter struct. I think those 3 should go in the device descriptor.

missing space before {

I would split this check in 2: one for the trigger pin, one for the echo pin

I don't think this comment is very useful. Better detail the mentioned protocol ? or give a link to a datasheet.

Missing closing ```
Or just indent the block with 4 spaces (and remove the opening ```). I would put the command line on a single line and remove -j2

missing space after while

Use puts without \n

Here, I think it would be better to extend the driver API with a srf04_read_distance that return the mesure in mm. The computation seems weird (x * 100 / 292 / 2). And better define constants in the driver header instead of hard coded values.

thie variable is not used, right?!

we could simply use dev->distance for keeping the state:

if (dev->distance > SRF04_MEASURING) {
    .... // start new measurement
}
else {
    ... // save actual distance time
} 

I suggest a small change here, which would allow us to keep state using the distance variable, and using these values as return codes as well:

enum {
    SRF04_OK = 0,
    SRF04_INVALID = -1,
    SRF04_MEASURING = -2,
}; // no need for typing this

and simply append the error codes from above with -3, -4...

add something like

@return              time of flight in ms (>= SRF04_OK)
@return  SRF04_MEASURING if measurement is in progress
@return SRF04_INVALID if no valid measurement is available

same here.

I think I would simplify here and drop the explicit ERR_INT value in favor of returning SRF04_ERR_GPIO in both cases.

maybe it makes sense to add a check so we don't trigger a new measurement while one is in progress:

if (dev->distance == SRF04_MEASURING) {
    return;
}

here we should also add a check:

if (dev->distance >= SRF04_OK) {
    return ((dev->distance * 100) / SRF04_DISTANCE);
}
return dev->distance;

with my proposed changes, it could be nice to check the return value of srf04_read_distance() and print an explicit error message in case the value is not valid (val < SRF04_OK).

last proposal: how about a convenience function that combines _trigger() and read_distance() together with the mandatory wait time?! This could be useful for very simple use cases (as e.g. the example application below...).

This limits the driver to a single hardware sensor. I suggest adding an argument to the init function, passing a pointer to a params struct. See other device drivers in the tree, for example https://github.com/RIOT-OS/RIOT/blob/master/drivers/io1_xplained/io1_xplained.c#L37-L39

This is not needed anymore with this added in the driver module dependencies.

Use include ../Makefile.tests_common instead of that line.

The attributes of this struct are not documented, same for the srf04_t struct below.

Maybe align a bit the values and comments

This check is weird since the function can return a distance or error codes (negative values). Maybe just compare to 0 ? Or use a switch and address each potential error code ?

Comparing to SRF04_OK is the same thing, since the enum defines it as zero and all errors would evaluate negative. I could distinguish between errors, but i deem it not necessary.

I thought about your concern and now i believe to understand what you meant. Distance should only be SRF04_OK (zero), when srf04_init returns successfully. After the first get_distance (wich happens before) it will either have a valid distance (greater or equal to 0) or give an error. So comparing to SRF04_OK is just meant to be a clean way of checking for new data.
As the sensor only measures values from a minimum distance of i think 3 cm it should always be greater then zero when successfull.

I'd say the check is fine. Maybe more intuitive would simply be to test for distance < 0, semantically considering the 0 as distance value instead of some arbitrarily defined named value. But I'd say this is up to the creative freedom of the author :-)

as discussed offline: drop the support for inches and move this define into the source file

this is static and has nothing to do with the interface -> move into source file

I would add a comment about the blocking nature of this function (will only return after the new value is acquired, which takes ~50ms..).

duplicate newline :-)

scope: cppcheck will probably complain that distance should be define inside the while loop below

for demonstration purposes, it is good practice to check the return value of the init function, e.g.

if (srf04_init(dev, params) != SRF04_OK) {
  puts("error initializing...");
  return 1;
}

or similar..

I'd say the check is fine. Maybe more intuitive would simply be to test for distance < 0, semantically considering the 0 as distance value instead of some arbitrarily defined named value. But I'd say this is up to the creative freedom of the author :-)