Implement 1wire bus.
(also includes mfifles patches not yet in the tree)
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| share/man/man4/ow_temp.4 | ||
|---|---|---|
| 81 | Needs serial comma: | |
| 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": | |
| 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": | |
| 13 | Maybe use own(4) to make this not seem to be the word "own". | |
| 15 | "to implement" is not needed. | |
| 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: | |
| 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.)
| |
| 50 | "The" needed: | |
Comment Actions
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.6875I 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): | |
Comment Actions
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 | |
| 64 | Avoid the aside: might have different operating characteristics like accuracy or | |
| 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 | |
| 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 just .Fn OWLL_READ_DATA 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 | |
| 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, | |
| 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 (Later flags are described with sentences ending in a period, so I added that here too.) | |
| 130 | Probably: | |
| 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 | |
| 148 | Again, recommend losing the redundant "The function" wording and pause: .Fn own_send_command | |
| 150 | "The" not needed. | |
| 153 | Trailing preposition... but let's see if the alternative is any better: .Fa pdev Hmm, not bad. | |
| 154 | "The" and "argument" are not needed. .Fa cmd | |
| 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 | |
| 169 | The final "to the bus" is not needed: presentation layer devices can use to coordinate access. | |
| 170 | s/The locking/Locking/ Locking is purely advisory at present. | |
| 173 | Eliminating the "The ... function" and going passive->active: .Fn own_acquire_bus | |
| 175 | We're talking about the function already, so I'm inclined to use "It" here. And passive->active: It waits indefinitely if | |
| 186 | Simplify this sentence, removing the "The ... function": .Fn own_release_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. | |
Comment Actions
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!
Comment Actions
Update based on Warren Block's feedback.
Didn't do everything but should have done most things.
Comment Actions
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.)
Comment Actions
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). | |
| 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:
| |
| 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. | |
Comment Actions
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?
Comment Actions
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.