Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Dec 20, 2019

While attempting to port to the new GPIO API I found the sample to be highly unreliable: The Adafruit breakout does not in fact have pull-ups on the alert signal, only on the I2C signals. A variety of other problems were identified, and resolving those issues resulted in a near complete rework:

  • Correct handling of device encoded temperature values, which combine a 12-bit 2s complement signed value with a separate sign bit. Rework conversion between device and sensor temperature representations to support negative temperatures in both domains.
  • Use a much simpler trigger configuration where the alert is driven by comparator output, rather than as an interrupt that requires a pair of I2C transactions to read and clear the flag.
  • Move to storing devicetree configuration data in flash, which removes ultra-long identifiers making the code more readable.
  • Refactor the trigger infrastructure to use the setup/handle/process idiom, which reduces duplicated code and to correctly detect alerts present when the triggers are set.
  • Completely replace the sample with something that demonstrates updating upper and lower threshold values to track moving temperatures.

Tested with all three trigger options, and with temperatures below zero Celsius.

@zephyrbot
Copy link

zephyrbot commented Dec 20, 2019

All checks passed.

checkpatch (informational only, not a failure)

-:527: WARNING:LONG_LINE: line over 80 characters
#527: FILE: drivers/sensor/mcp9808/mcp9808_trigger.c:201:
+					GPIO_INT_ACTIVE_LOW | GPIO_INT_DEBOUNCE);

-:535: WARNING:LONG_LINE: line over 80 characters
#535: FILE: drivers/sensor/mcp9808/mcp9808_trigger.c:205:
+		gpio_init_callback(&data->alert_cb, alert_cb, BIT(cfg->alert_pin));

- total: 0 errors, 2 warnings, 776 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc changes in #21427 should win

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc changes in #21427 are preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates made (plus-minus, explanation of interleaved output). Please continue documentation review here; #21427 will be rebased on top of this change when it goes in.

@pabigot
Copy link
Contributor Author

pabigot commented Jan 3, 2020

@dbkinder Please revisit this, all documentation change requests here and from #21427 should be addressed. Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: C instead of Cel, all other samples use C.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other samples are "wrong" because the unit is degrees Celsius, not coulombs, but I'll change this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)
some sample actually have °C, but those have some kind of display.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, interleaved output from unsynchronized printf from thread and ISR.

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested with sensor breakout on disco_l475_iot1, works for me.

Correct handling of device encoded temperature values, which combine a
12-bit 2s complement signed value with a separate sign bit.  Rework
conversion between device and sensor temperature representations to
support negative temperatures in both domains.

Use a much simpler trigger configuration where the alert is driven by
comparator output, rather than as an interrupt that requires a pair of
I2C transactions to read and clear the flag.

Refactor the trigger infrastructure to use the setup/handle/process
idiom, which reduces duplicated code and to correctly detect alerts
present when the triggers are set.

Completely replace the sample with something that demonstrates
updating upper and lower threshold values to track moving
temperatures.

Signed-off-by: Peter A. Bigot <[email protected]>
@pabigot pabigot force-pushed the pabigot/20191220a branch from 22c62e3 to 5f08738 Compare January 8, 2020 20:50
@pabigot
Copy link
Contributor Author

pabigot commented Jan 8, 2020

Changes complete.

@nashif nashif merged commit f05cbb4 into zephyrproject-rtos:master Jan 9, 2020
@pabigot pabigot deleted the pabigot/20191220a branch January 16, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants