Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify concepts section #348

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Clarify concepts section #348

merged 1 commit into from
Mar 1, 2018

Conversation

anssiko
Copy link
Member

@anssiko anssiko commented Mar 1, 2018

  • Use singular form for definitions
  • Move extensibility related prose to Extensibility section
  • Link to 'reading timestamp' concept from Sensor.timestamp section
  • Remote redundant definitions
  • Misc editorial changes

Preview | Diff

Copy link

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

There are plural => singular changes, maybe text needs to be aligned accordingly. @anssiko @pozdnyakov wdyt?

Otherwise, looks like a good improvement 👍

index.bs Outdated
The [=device sensors=] measure different physical quantities
and provide corresponding <dfn>raw sensor readings</dfn>
which are a source of information about the user and their environment.
A [=device sensor=] measures different physical quantities

Choose a reason for hiding this comment

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

Before it was "sensors => physical quantities", if we change to singular, should it be "sensor => physical quantity"

Choose a reason for hiding this comment

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

and why to emphasize different physical quantities?

Choose a reason for hiding this comment

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

"different" could be dropped :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated
Each [=raw sensor reading|reading=] is composed of the <dfn lt="reading value">values</dfn>
of the different physical quantities measured by the [=device sensor|sensor=]
Each [=sensor reading=] is composed of the <dfn lt="reading value">values</dfn>
of the different physical quantities measured by the [=device sensor=]

Choose a reason for hiding this comment

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

Same as above, unclear why to use different physical quantities rather than physical quantity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated

If the [=sensor readings=] represent spatial measurements (e.g.
acceleration, angular velocity), they must be resolved in
If a [=sensor reading=] represents spatial measurements (e.g.

Choose a reason for hiding this comment

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

sensor reading => measurement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated
@@ -578,6 +564,11 @@ but if the provided [=sensor readings=] are product of [=sensor fusion=] perform
in software, the [=platform sensor=] corresponds to a set of [=device sensors=]
involved in the [=sensor fusion=] process.

Discrepancies between a [=sensor reading=]
and the corresponding physical quantities being measured

Choose a reason for hiding this comment

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

physical quantity?

Choose a reason for hiding this comment

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

could it be several "quantities" for the same sensor? (e.g. acceleration and angular velocity for orientation sensor)

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving this as is.

index.bs Outdated
@@ -1526,6 +1517,13 @@ Such <dfn lt="extension specification">extension specifications</dfn> are encour
single [=sensor type=], exposing both [=high-level|high=] and [=low-level|low=] level
as appropriate.

[=Extension specifications=] are encouraged to define whether a [=calibration=]
process applies to <a>sensor readings</a>.

Choose a reason for hiding this comment

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

nit: [=sensor readings=]

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@pozdnyakov
Copy link

Looks good overall

@anssiko
Copy link
Member Author

anssiko commented Mar 1, 2018

Thanks for the review! I've addressed your comments in 4562f95

I also regenerated using the bikeshed HTTP API (rather than locally as be usually):

curl https://api.csswg.org/bikeshed/ -F file=@index.bs -F force=1 > index.html

And still there's index.html conflict. Ideas?

index.bs Outdated
The [=device sensors=] measure different physical quantities
and provide corresponding <dfn>raw sensor readings</dfn>
which are a source of information about the user and their environment.
A [=device sensor=] measures a physical quantity

Choose a reason for hiding this comment

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

a device sensor can measure several physical quantities (e.g. orientation sensor), so I'd keep plural here..

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated

If the [=sensor readings=] represent spatial measurements (e.g.
acceleration, angular velocity), they must be resolved in
If a <dfn>measurement</dfn> represents a spatial measurement (e.g.

Choose a reason for hiding this comment

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

maybe better "If the sensor performs spatial measurements"

Choose a reason for hiding this comment

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

the "measurement" definition here does not seem to explain that term, do we need this definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the dfn, people can refer to https://en.wikipedia.org/wiki/Measurement

index.bs Outdated
a <dfn export>local coordinate system</dfn> that represents
a reference frame for these [=sensor readings|readings=].
A [=device sensor|sensor=] that provides such [=sensor readings|readings=]
a reference frame for these [=sensor readings=].

Choose a reason for hiding this comment

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

now "these" does not fit, maybe "for the sensor's [=sensor readings=]"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@anssiko
Copy link
Member Author

anssiko commented Mar 1, 2018

Addressed @pozdnyakov's remaining comments, squashed all the review comments from this PR into 55e3ee6 and rebased that fixed the merge conflict.

- Use singular form for definitions
- Move extensibility related prose to Extensibility section
- Link to 'reading timestamp' concept from Sensor.timestamp section
- Remove redundant definitions
- Misc editorial changes
@anssiko anssiko merged commit 5d55914 into master Mar 1, 2018
@anssiko anssiko deleted the concepts branch March 1, 2018 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants