Page MenuHomeFreeBSD

1 wire stuff: basics and temperature
Needs ReviewPublic

Authored by imp on Jun 30 2015, 2:24 PM.

Details

Reviewers
stas
wblock
loos
Group Reviewers
manpages
Summary

Implement 1wire bus.

(also includes mfifles patches not yet in the tree)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
wblock added a comment.Jul 7 2015, 8:48 PM

Some of these might be duplicates that have been fixed. Phabricator is very confused about this, but did not let that stop it from showing it as two utterly unrelated diffs.

Sorry about that. Next time, I'll just compare the raw diffs.

share/man/man4/ow_temp.4
81

Needs serial comma:
s/issues/issues,/

84

"counting from CRC errors" is still unclear. I originally thought this meant "the number of non-CRC errors." Not so sure now.

91

"may" is generally used for permission. "might" or "can" is better for probability:

It can also indicate a wiring error.

113

Low and high temperature... what? Limits? Ranges? Alarms? "either" is probably not needed.

share/man/man4/owc.4
37

Remove "the":
module implements Dallas Semiconductor 1-wire signaling.

45

Phabricator might be helping, but this shows "The only a controller" now. Should be "The only controller"?

46

Suggestion: use "with" rather than "via" ("via" implies travel: "I got there via Highway 8" or a transfer medium: "Electricity travels via a conductor.")

pins on a GPIO bus with

68

Should be either "GPIO" or .Xr gpio 4

sys/dev/ow/README.txt
12

Missing a "the":
s/in ow/in the ow/

13

Maybe use own(4) to make this not seem to be the word "own".

15

"to implement" is not needed.
responsibility of the client device drivers.

17

As above, own(4) rather than just "own". (It is easy to read this as a type saying "their own".

sys/dev/ow/owc_gpiobus.c
318

Typo:
s/interferring/interfering/

sys/dev/ow/owll_if.m
43

For this version of the sentence, "has" is correct. (Sorry if this is a duplicate, or changing it back. Phabricator is "helping" the crap out of this, and it's hard to tell what has changed since the original and new version are not shown together.)

  1. SoCs list a 1-wire controller in their spec sheets which presumably has
50

"The" needed:
s/owll/the owll/

125

Typo:
s/resitor/resistor/

imp updated this revision to Diff 6763.Jul 7 2015, 10:33 PM

More doc edits from Warren

Only a couple of minor changes. Otherwise, looks pretty good!

share/man/man4/ow_temp.4
83

Typo:
s/an/a/

share/man/man4/owc.4
45

Typo:
s/Big/Bit/

sys/dev/ow/README.txt
18

s/the own/own/

But other than that, nice!

sys/dev/ow/owll_if.m
43

Even better!

imp updated this revision to Diff 6852.Jul 11 2015, 1:01 AM

Update to include own.9 and owll.9 man pages.

loos accepted this revision.Jul 11 2015, 5:41 PM
loos edited edge metadata.

Tested with Banana Pi and one DS1820 (or maybe DS18S20):

owc0: <FDT GPIO attached one-wire bus> at pin(s) 275 on gpiobus0
ow0: <1 Wire Bus> on owc0
ow_temp0: <One Wire Temperature> romid 10:20:26:b6:00:08:00:c7 on ow0
# sysctl dev.ow_temp
dev.ow_temp.0.parasite: 0
dev.ow_temp.0.reading_interval: 1000
dev.ow_temp.0.badread: 0
dev.ow_temp.0.badcrc: 14
dev.ow_temp.0.temperature: 363
dev.ow_temp.0.%parent: ow0
dev.ow_temp.0.%pnpinfo: romid=10:20:26:b6:00:08:00:c7
dev.ow_temp.0.%location: 
dev.ow_temp.0.%driver: ow_temp
dev.ow_temp.0.%desc: One Wire Temperature
dev.ow_temp.%parent:
# sysctl dev.ow_temp.0.temperature | cut -f 2 -d: | awk '{ printf "%.4f\n", $1 / 16 }'
22.6875

I also added the following to build the 1-wire support in kernel (not as a module):

Index: sys/conf/files
===================================================================
--- sys/conf/files      (revision 285250)
+++ sys/conf/files      (working copy)
@@ -2014,6 +2014,13 @@
 dev/ofw/ofwbus.c               optional fdt
 dev/ofw/openfirm.c             optional fdt
 dev/ofw/openfirmio.c           optional fdt
+dev/ow/ow.c                    optional ow                             \
+       dependency      "owll_if.h"                                     \
+       dependency      "own_if.h"
+dev/ow/owll_if.m               optional ow
+dev/ow/own_if.m                        optional ow
+dev/ow/ow_temp.c               optional ow_temp
+dev/ow/owc_gpiobus.c           optional owc gpio
 dev/patm/if_patm.c             optional patm pci
 dev/patm/if_patm_attach.c      optional patm pci
 dev/patm/if_patm_intr.c                optional patm pci

All the CRC errors I've seen so far are '0' that are read as '1':

ow_temp0: ow_temp_event_thread: scratch reg: 2b:00:4b:46:ff:ff:03:10:4e == crc: 0x4e (CRC match)
ow_temp0: ow_temp_event_thread: scratch reg: 2b:00:4b:46:ff:ff:02:10:8a == crc: 0x8a (CRC match)
ow_temp0: ow_temp_event_thread: scratch reg: 2b:02:4b:46:ff:ff:02:10:8a == crc: 0xf0
                                                ^^
ow_temp0: ow_temp_event_thread: scratch reg: 6b:00:4b:46:ff:ff:03:10:4e == crc: 0x7
                                             ^^
ow_temp0: ow_temp_event_thread: scratch reg: 2b:00:4b:47:ff:ff:03:10:4e == crc: 0x83
                                                      ^^
ow_temp0: ow_temp_event_thread: scratch reg: 2b:00:6b:46:ff:ff:06:10:b1 == crc: 0x7
                                                   ^^

Do you think we need to GETBUS() and RELBUS() for each bit sent or received ? I think this is okay because it is the master that controls the slave timing, so this won't break the transfer if another child takes the bus between two bits.

This was very well thought, thank you for making this happen.

sys/dev/ow/owc_gpiobus.c
56–57

Check #define<tab> vs #define<space> occurrences.

292

Reducing this delay to (t->t_rdv / 4) seems to help with CRC errors (but not enough to eliminate them).

332

extra blank line.

sys/dev/ow/owll_if.m
31

When building with the 1-Wire support in kernel, I need add (to include the struct ow_timing definition):
#include <dev/ow/owll.h>

This revision is now accepted and ready to land.Jul 11 2015, 5:41 PM

Again, Phabricator is showing these as completely deleting the old version of the file and then completely adding the new one. That makes it very hard to tell what has changed, so please pardon fragmentary comments that might overlap old ones.

There might be a way to submit the diff that would work better. I don't know, I use the web interface to avoid installing PHP.

share/man/man4/ow_temp.4
43

s/The following/These

61

Avoid redundancy by avoiding redundancy (see also: redundancy). Probably not necessary to capitalize "family codes":

The family codes for these devices are reused by many variants
marketed under different names.

64

Avoid the aside:

might have different operating characteristics like accuracy or
temperature range.

66

Rearrange to avoid the repetitive "This driver". Also add a serial comma:

Family codes 0x10, 0x22, 0x28, and 0x3b are supported.

68

"Supports" is not really the right word here. The driver *provides* those entries. Probably better to use .Nm than "this driver", too:

.Nm
provides several entries in the device's node in the
.Xr sysctl 8
tree:

share/man/man9/owll.9
50

Probably should say "device" or "driver" here in this opening sentence.

device interfaces to the link layer of the Dallas Semiconductor 1-Wire.

59

It is less repetitive and easier to read (IMO) if "The blahblah function is" is replaced with just "blahblah is". There was some contention about this, as you might recall. I've been assured that I am worse than that one bad guy for thinking this.

So:

.Fn OWLL_WRITE_ONE
​and
​.Fn OWLL_WRITE_ZERO
write a...

and just

.Fn OWLL_READ_DATA
​reads one...

This can be repeated all through this page for a shorter and clearer result.

77

Try to avoid the informal "you":

Rather than calling the
.Nm
interface directly, consider augmenting the
.Xr own 9
interface to expose the needed functionality instead.

share/man/man9/own.9
71

Missing "s", extra word:

These functions are split into three groups:

79

A little vague here. "Should" used as a recommendation is fine. "Generally" gets a little questionable. As opposed to what other options? Why?

Something about the wrappers providing safer or more general-purpose usage?

81

Probably can replace "to the devices on the bus" with just "to devices.":

The 1-Wire bus defines different layers of access to devices.

85

"of the bus" is not needed:

functions provide access to the network and transport layers.

90

Trailing preposition. Some people say these are fine, but they often indicate that the sentence can be rearranged to make more sense. Not sure on this. Does the network layer "specify" (as in choose) the clock speed of transport layer commands, or do transport layer commands just work at whatever rate the network layer is set to use? In other words, can transport layer commands run at a different clock rate than the network layer? I'd guess not, and guess that it really means this:

Transport layer commands run at a clock rate (speed?) determined by the network layer.

94

Rearrange to avoid nonspecific "it" when possible. Also, serial comma:

Network access, speed, and timing information are encapsulated in this structure,
as are commands to send and whether or not data is read.

96

s/files/fields/ (although this came up the other day somewhere else, C actually calls these "members" of a struct)

99

"Various" is not needed.

Flags to control the interpretation of the structure.

100

These flags are defined in

104

ROM is capitalized later on, should be here also.

106

This is... confusing. It is hard to tell the groupings. "Send Transport" seems like a description, not a verb, but hard to tell. Is "stuff" a noun or a verb? If this is just a single flag, it might help to start with "Enable" or "Set". Some quotes on the pairings of words could help, too:

Enable
.Dq Send Transport
(XPT) overdrive speed.

(Later flags are described with sentences ending in a period, so I added that here too.)

130

Probably:
s/send/sent/

134

Needs a comma for the pause:

.Va flags ,

135

"then" is not needed.

it is the number of bits to read.

143

Avoid the "The blah function" and use just a comma rather than the aside. Also, use active voice:

.Fn own_command_wait
acquires the 1-Wire bus, waiting if necessary,
sends the command,
then releases the bus.

148

Again, recommend losing the redundant "The function" wording and pause:

.Fn own_send_command
sends the command without bus reservation.

150

"The" not needed.

153

Trailing preposition... but let's see if the alternative is any better:

.Fa pdev
is the client, presentation layer device, on whose behalf the command is sent.

Hmm, not bad.

154

"The" and "argument" are not needed.

.Fa cmd
describes the transaction to send to the 1-Wire bus.

158

"The" not needed.

160

"the ... argument" is not needed. But some rewriting to avoid this long sentence should be done:

.Fn own_self_command
fills in
.Fa cmd
with a MATCH_ROM command.
This includes the ROM address of
.Fa pdev
and
.Fa xpt_cmd
as a convenient way to create directed commands.

169

The final "to the bus" is not needed:

presentation layer devices can use to coordinate access.

170

s/The locking/Locking/
s/at this time/at present/

Locking is purely advisory at present.

173

Eliminating the "The ... function" and going passive->active:

.Fn own_acquire_bus
reserves the bus.

175

We're talking about the function already, so I'm inclined to use "It" here. And passive->active:

It waits indefinitely if
.Fa how
is
.Dv OWN_WAIT
and returns with the error
.Dv EWOULDBLOCK
if passed
.Dv OWNDONTWAIT
when the bus is owned by another client.

186

Simplify this sentence, removing the "The ... function":

.Fn own_release_bus
releases the bus.

196

Nice! Easier to read without saying "the buffer argument" and "the len argument".

200

Probably can remove a "the":

the timing of signaling of different modes, and the polling

Could be clearer if the multiple "of"s could be simplified ("timing of signalling of different modes"). Maybe break into separate sentences.

204

"is" is not quite right here. Maybe:

presentation layer is composed of the device-specific commands and actions used to

205

"the" not needed here, nor is "on bus":

control specific 1-Wire devices.

212

Guidelines say we should always call them "manual pages", but here we can probably do better:

device) should only call the functions described here.

imp added a comment.Jul 13 2015, 4:44 AM
In D2956#60234, @loos wrote:

Tested with Banana Pi and one DS1820 (or maybe DS18S20):
Index: sys/conf/files

Added, thanks.

All the CRC errors I've seen so far are '0' that are read as '1':

This suggests that we're polling late. Perhaps DELAY is DELAYING too long?

I'm away from my hardware right now, but I was thinking of getting the time before we drive the line low, drive the line low and let it float high and start polling and compute a the time after we see it float high to figure out if it is a zero or one. At least as a debugging process, if not for actual operation. It's a busy wait either way...

Do you think we need to GETBUS() and RELBUS() for each bit sent or received ? I think this is okay because it is the master that controls the slave timing, so this won't break the transfer if another child takes the bus between two bits.

I don't think that we need to do this. The enumerate code is effectively single threaded.

This was very well thought, thank you for making this happen.

You're welcome. Thanks for testing!

imp updated this revision to Diff 6947.Jul 15 2015, 3:52 AM
imp edited edge metadata.

Update based on Warren Block's feedback.

Didn't do everything but should have done most things.

This revision now requires review to proceed.Jul 15 2015, 3:52 AM

Phabricator again has made it impossible to compare original with new. How about if I download the raw diff, edit the files, then send you a resulting diff? (Sorry.)

loos added a comment.Jul 16 2015, 1:33 PM

Found a double free() while trying the modules.

How to reproduce:

kldload ow_temp
kldload owc

And I need this include to fix a build error (missing definition of struct ow_timing) :

--- sys/dev/ow/owll_if.m.orig      2015-07-15 21:53:38.524415000 -0300
+++ sys/dev/ow/owll_if.m   2015-07-15 21:53:46.177856000 -0300
@@ -27,6 +27,7 @@
 #
 
 #include <sys/bus.h>
+#include <dev/ow/owll.h>
 
 INTERFACE owll;
sys/dev/ow/owc_gpiobus.c
136

Double free.

The kids variable is already freed on line 132 (this line can be removed).

imp added inline comments.Jul 21 2015, 5:17 AM
sys/dev/ow/ow.c
320

Experience has shown that this loop is pathological when talking to a naked bus.

We get a count of all the possible address IDs, most of which have CRC errors.

At effectively SPLHIGH().

This must be mitigated somehow.

Some suggestions:

  1. add some kind of 'ensure the bus is idle' callback to the owll layer.
  2. make sure the owll layer signals read errors by making sure the line eventually goes high and erroring out somehow here.
  3. Profit!
darius-dons.net.au added inline comments.
share/man/man4/ow.4
37

"implements the", or "implements a"

sys/dev/ow/owll_if.m
148

What are the return code(s)?
Add one for 'bus held low' i.e. delay for G and read, if it's low bail out.

imp added inline comments.Jul 21 2015, 5:28 AM
sys/dev/ow/owll_if.m
148

That's a good question. And a good suggestion for dealing with the 'stuck low' issue. I'll document some convention here.

imp added a comment.Jul 21 2015, 5:24 PM
In D2956#61538, @loos wrote:

Found a double free() while trying the modules.

How to reproduce:

kldload ow_temp
kldload owc

And I need this include to fix a build error (missing definition of struct ow_timing) :

--- sys/dev/ow/owll_if.m.orig      2015-07-15 21:53:38.524415000 -0300
+++ sys/dev/ow/owll_if.m   2015-07-15 21:53:46.177856000 -0300
@@ -27,6 +27,7 @@
 #
 
 #include <sys/bus.h>
+#include <dev/ow/owll.h>
 
 INTERFACE owll;

but owll.h includes owll_if.h which creates a circular dependency, no? only owll.h should be used. What's the compile error without this?

imp updated this revision to Diff 7213.Jul 23 2015, 5:44 AM
imp edited edge metadata.

Latest fixes

wblock accepted this revision.Jul 23 2015, 2:11 PM
wblock added a reviewer: wblock.

The man pages look good to me. Thanks!

This revision is now accepted and ready to land.Jul 23 2015, 2:11 PM
imp updated this revision to Diff 7242.Jul 23 2015, 9:35 PM
imp edited edge metadata.

Go to oversampling the read pulses. This increases accuracy, slightly,
at the cost of making the code more complicated.

Poll for the rest pulse a little later to avoid some false positives
that were seen. We have a huge window to poll in, so use it. I thought
about going to the oversampling approach here as well, but the code
was ickier and didn't produce better results.

Start reporting in milliKelvin. Changes to sysctl are needed for this
to work.

This revision now requires review to proceed.Jul 23 2015, 9:35 PM