Skip to content

Conversation

@smlng
Copy link
Member

@smlng smlng commented Jun 27, 2017

based on #7193

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 27, 2017
@smlng smlng requested review from haukepetersen and miri64 June 27, 2017 09:52
@kaspar030
Copy link
Contributor

based on #7193

please state:

  • what does this PR do
  • how does it do it

@smlng
Copy link
Member Author

smlng commented Jun 27, 2017

This PR adapts scale to string conversion to work with multi dimensional units like square (m^2) and cubic meters (m^3). For further info see discussion in #7193

@aabadie aabadie modified the milestone: Release 2017.07 Jun 27, 2017
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 28, 2017
@miri64
Copy link
Member

miri64 commented Jun 28, 2017

#7193 was merged, please rebase.

@smlng smlng force-pushed the sys/phydat/enh_scale2str branch from 650d65c to a1d0c7e Compare June 28, 2017 12:48
@smlng
Copy link
Member Author

smlng commented Jun 28, 2017

rebased

@smlng
Copy link
Member Author

smlng commented Jun 28, 2017

another option would be to leave scale_to_str as is and make the check and rescaling for certain units in the calling function phydat_dump.

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

partly NACK: IMHO we can simplify to the following:

char phydat_scale_to_str(int8_t scale, uint8_t unit)
{
    switch (unit) {
        case UNIT_M2:
        case UNIT_M3:
        case UNIT_TEMP_C:
        /* ... add all others for which scale names don't make sense */
        return '\0';
    }
    
    switch (scale) {
...

Also, the calls to phydat_str need to be adapted...

@miri64
Copy link
Member

miri64 commented Jun 28, 2017

another option would be to leave scale_to_str as is and make the check and rescaling for certain units in the calling function phydat_dump.

Yes, alternatively this check can be done in phydat_dump(). What do you think @haukepetersen?

@smlng
Copy link
Member Author

smlng commented Jun 28, 2017

would be less intrusive, too - I guess

@smlng
Copy link
Member Author

smlng commented Jun 28, 2017

I changed the handling as suggested

@miri64
Copy link
Member

miri64 commented Jun 28, 2017

I changed the handling as suggested

Now the PR needs a new title ;-)

@smlng
Copy link
Member Author

smlng commented Jun 28, 2017

depends: which version do you and @haukepetersen like better?

case UNIT_PERCENT:
case UNIT_TEMP_C:
case UNIT_TEMP_F:
case UNIT_TEMP_K:
Copy link
Member

Choose a reason for hiding this comment

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

Afaik Kelvin (a SI unit) my be used with SI prefix.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

though completely unrealistic for RIOT, but fine - I'll remove it

Copy link
Member

Choose a reason for hiding this comment

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

Well it's one check less saving a few byte of ROM ;-)

@smlng smlng changed the title sys, phydat: enhance scale_to_string conversion sys, phydat: omit scale to string conversion if not applicable Jun 28, 2017
@haukepetersen
Copy link
Contributor

I don't like to put it into phydat_dump, as the phydat_scale_to_str() is/will be used also in other contexts. So I really prefer the 'white-listing' approach for simply returning \0 for a number of units.

@smlng
Copy link
Member Author

smlng commented Jul 2, 2017

@haukepetersen currently phydrat_scale_to_str() simply does what it says, without any dependencies, which I think is good. The additional check for certain units would require to pass the unit to that function, which I think would change its purpose.

IMHO for now its fine to have it in phydrat_dump, we may at an intermediate function later on - who is using scale_to_str, too?

@miri64 miri64 mentioned this pull request Jul 3, 2017
@haukepetersen
Copy link
Contributor

ok, fine with me. Then lets keep it like this.

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Aug 25, 2017
@haukepetersen
Copy link
Contributor

please squash

@smlng smlng force-pushed the sys/phydat/enh_scale2str branch from 129d738 to 5bbafec Compare August 25, 2017 10:26
@smlng smlng removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 25, 2017
@smlng
Copy link
Member Author

smlng commented Aug 25, 2017

done

@haukepetersen
Copy link
Contributor

Could you quickly fix the whitespace issues murdock is complaining about? Just squash the fixes right in...

@smlng smlng force-pushed the sys/phydat/enh_scale2str branch from 5bbafec to 93c0cfd Compare August 25, 2017 12:08
@smlng
Copy link
Member Author

smlng commented Aug 25, 2017

whitespace issues

damn, my bad - anyway, fix amended.

@haukepetersen
Copy link
Contributor

Murodck is still unhappy, but the cause could be unrelated to your changes. Would you fix it anyway? Thx

@smlng smlng force-pushed the sys/phydat/enh_scale2str branch from 93c0cfd to e4382ae Compare August 25, 2017 12:39
@smlng
Copy link
Member Author

smlng commented Aug 25, 2017

I think I made non-commit, with[out] actually pushing the fix - let's see what Murdock says now

@haukepetersen
Copy link
Contributor

All green -> lets go

@haukepetersen haukepetersen merged commit 7f14e15 into RIOT-OS:master Aug 25, 2017
@smlng smlng deleted the sys/phydat/enh_scale2str branch August 25, 2017 16:08
@jnohlgard jnohlgard added the Area: SAUL Area: Sensor/Actuator Uber Layer label Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: SAUL Area: Sensor/Actuator Uber Layer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants