Re: [riot-devel] Byte array should be uint8_t, not char

[Kees Bakker]

Hi Joakim,

I've done a PR for SPI. https://github.com/RIOT-OS/RIOT/pull/5901
If you have a chance, can you have a look at it/

What this set of changes shows is that several developers have been
inserting casts to be able to get SPI code compile without errors. These 
casts

(from uint8_t to char) are now not needed anymore.

Yes, there were quite a few files to be modified, but the changes were 
straightforward.
I even patched a patch file :-) See 
pkg/u8g2/patches/0002-u8g2-add-riot-os-interface.patch

-- Kees

On 17-07-16 18:40, Joakim Nohlgård wrote:

Hi Kees,
Do you have the time/energy to resume the effort of updating the 
periph/spi interface in where Hauke left off in 
https://github.com/RIOT-OS/RIOT/pull/4780?


There are a lot of device drivers which need updating, and also some 
existing comments on the PR.


I can assist in reviewing the changes and updating device drivers, but 
I don't have a lot of time to spend on this right now.


Best regards,
Joakim

On Wed, Jul 6, 2016 at 7:00 AM, Ludwig Knüpfer 
> 
wrote:


Hi Kees,

I assume there are more violations than the ones in the SPI
drivers... But of course every step forward is great!

Testing can be done by the community. We have plans to create a
distributed test system with actual hardware attached, but sadly
that has not become reality yet.

Compilation can be tested automatically by running `make
buildtest` for the respective test application (tests/periph_spi).

Cheers,
Ludwig

Am 5. Juli 2016 21:31:49 MESZ, schrieb Kees Bakker >:
>Hi Ludwig,
>
>Well, it will be a challenge to smootly correct this.
>There are 16 CPU's that use spi_transfer_byte(s) and 6 drivers.
>
>I won't mind creating a PR, but of course I can only test it by
>building
>examples for all boards that support SPI. And look at compile errors.
>Or are
>there other procedures?
>
>On 04-07-16 07:23, Ludwig Knüpfer wrote:
>> Hi Kees,
>>
>> Unless there is a good reason to deviate from this guideline all
>violations should be corrected. This particular rule was added
>relatively recently, so it would not surprise me if not all
occurrences
>in RIOT have been adapted yet.
>>
>> Cheers,
>> Ludwig
>>
>> Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker
>:
>>> Hi,
>>>
>>> The Coding Convention is clear about it.
>>>
>>> "Guidelines for pointer types (as long as it is reasonable):
>>>
>>>   * use |char *| for strings and only for strings
>>> * use |uint8_t[]| as type for arbitrary byte buffers, but use
|void
>*|
>>> to pass them around. |uint8_t[]| because we're dealing with
>bytes
>>> and not characters, |void *| to avoid unnecessary casting
shall
>the
>>> need arise to have their content to have a certain type
>>>   * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
>>> addresses, where it avoids unnecessary temporary variable
>>>   * use |void *| for generic typing"
>>>
>>>
>>> In the SPI driver however the transfer functions use char *
>parameters,
>>>
>>> but SPI is usually dealing with binary
>>> information (bytes), not strings. This leads to unnecessary
casts in
>>> other parts of the code. (E.g. nvram_spi).
>>>
>>> What is our policy about this? Are we going to correct this at
some
>>> point? Is it too late already (I hope not)?
>> ___
>> devel mailing list
>> devel@riot-os.org 
>> https://lists.riot-os.org/mailman/listinfo/devel
>
>
>--
>Kees Bakker
>Founder
>SODAQ
>M. 0031617737165
>www.sodaq.com 
>
>___
>devel mailing list
>devel@riot-os.org 
>https://lists.riot-os.org/mailman/listinfo/devel

___
devel mailing list
devel@riot-os.org 
https://lists.riot-os.org/mailman/listinfo/devel




___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel



--
Kees Bakker
Founder
SODAQ
M. 0031617737165
www.sodaq.com

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Byte array should be uint8_t, not char

[Joakim Nohlgård]

Hi Kees,
Do you have the time/energy to resume the effort of updating the periph/spi
interface in where Hauke left off in
https://github.com/RIOT-OS/RIOT/pull/4780?

There are a lot of device drivers which need updating, and also some
existing comments on the PR.

I can assist in reviewing the changes and updating device drivers, but I
don't have a lot of time to spend on this right now.

Best regards,
Joakim

On Wed, Jul 6, 2016 at 7:00 AM, Ludwig Knüpfer  wrote:

> Hi Kees,
>
> I assume there are more violations than the ones in the SPI drivers... But
> of course every step forward is great!
>
> Testing can be done by the community. We have plans to create a
> distributed test system with actual hardware attached, but sadly that has
> not become reality yet.
>
> Compilation can be tested automatically by running `make buildtest` for
> the respective test application (tests/periph_spi).
>
> Cheers,
> Ludwig
>
> Am 5. Juli 2016 21:31:49 MESZ, schrieb Kees Bakker :
> >Hi Ludwig,
> >
> >Well, it will be a challenge to smootly correct this.
> >There are 16 CPU's that use spi_transfer_byte(s) and 6 drivers.
> >
> >I won't mind creating a PR, but of course I can only test it by
> >building
> >examples for all boards that support SPI. And look at compile errors.
> >Or are
> >there other procedures?
> >
> >On 04-07-16 07:23, Ludwig Knüpfer wrote:
> >> Hi Kees,
> >>
> >> Unless there is a good reason to deviate from this guideline all
> >violations should be corrected. This particular rule was added
> >relatively recently, so it would not surprise me if not all occurrences
> >in RIOT have been adapted yet.
> >>
> >> Cheers,
> >> Ludwig
> >>
> >> Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker :
> >>> Hi,
> >>>
> >>> The Coding Convention is clear about it.
> >>>
> >>> "Guidelines for pointer types (as long as it is reasonable):
> >>>
> >>>   * use |char *| for strings and only for strings
> >>> * use |uint8_t[]| as type for arbitrary byte buffers, but use |void
> >*|
> >>> to pass them around. |uint8_t[]| because we're dealing with
> >bytes
> >>> and not characters, |void *| to avoid unnecessary casting shall
> >the
> >>> need arise to have their content to have a certain type
> >>>   * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
> >>> addresses, where it avoids unnecessary temporary variable
> >>>   * use |void *| for generic typing"
> >>>
> >>>
> >>> In the SPI driver however the transfer functions use char *
> >parameters,
> >>>
> >>> but SPI is usually dealing with binary
> >>> information (bytes), not strings. This leads to unnecessary casts in
> >>> other parts of the code. (E.g. nvram_spi).
> >>>
> >>> What is our policy about this? Are we going to correct this at some
> >>> point? Is it too late already (I hope not)?
> >> ___
> >> devel mailing list
> >> devel@riot-os.org
> >> https://lists.riot-os.org/mailman/listinfo/devel
> >
> >
> >--
> >Kees Bakker
> >Founder
> >SODAQ
> >M. 0031617737165
> >www.sodaq.com
> >
> >___
> >devel mailing list
> >devel@riot-os.org
> >https://lists.riot-os.org/mailman/listinfo/devel
>
> ___
> devel mailing list
> devel@riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel
>
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Byte array should be uint8_t, not char

[Ludwig Knüpfer]

Hi Kees,

I assume there are more violations than the ones in the SPI drivers... But of 
course every step forward is great!

Testing can be done by the community. We have plans to create a distributed 
test system with actual hardware attached, but sadly that has not become 
reality yet.

Compilation can be tested automatically by running `make buildtest` for the 
respective test application (tests/periph_spi).

Cheers,
Ludwig

Am 5. Juli 2016 21:31:49 MESZ, schrieb Kees Bakker :
>Hi Ludwig,
>
>Well, it will be a challenge to smootly correct this.
>There are 16 CPU's that use spi_transfer_byte(s) and 6 drivers.
>
>I won't mind creating a PR, but of course I can only test it by
>building
>examples for all boards that support SPI. And look at compile errors.
>Or are
>there other procedures?
>
>On 04-07-16 07:23, Ludwig Knüpfer wrote:
>> Hi Kees,
>>
>> Unless there is a good reason to deviate from this guideline all
>violations should be corrected. This particular rule was added
>relatively recently, so it would not surprise me if not all occurrences
>in RIOT have been adapted yet.
>>
>> Cheers,
>> Ludwig
>>
>> Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker :
>>> Hi,
>>>
>>> The Coding Convention is clear about it.
>>>
>>> "Guidelines for pointer types (as long as it is reasonable):
>>>
>>>   * use |char *| for strings and only for strings
>>> * use |uint8_t[]| as type for arbitrary byte buffers, but use |void
>*|
>>> to pass them around. |uint8_t[]| because we're dealing with
>bytes
>>> and not characters, |void *| to avoid unnecessary casting shall
>the
>>> need arise to have their content to have a certain type
>>>   * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
>>> addresses, where it avoids unnecessary temporary variable
>>>   * use |void *| for generic typing"
>>>
>>>
>>> In the SPI driver however the transfer functions use char *
>parameters,
>>>
>>> but SPI is usually dealing with binary
>>> information (bytes), not strings. This leads to unnecessary casts in
>>> other parts of the code. (E.g. nvram_spi).
>>>
>>> What is our policy about this? Are we going to correct this at some
>>> point? Is it too late already (I hope not)?
>> ___
>> devel mailing list
>> devel@riot-os.org
>> https://lists.riot-os.org/mailman/listinfo/devel
>
>
>-- 
>Kees Bakker
>Founder
>SODAQ
>M. 0031617737165
>www.sodaq.com
>
>___
>devel mailing list
>devel@riot-os.org
>https://lists.riot-os.org/mailman/listinfo/devel

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Byte array should be uint8_t, not char

[Ludwig Knüpfer]

Hi,

gebart is Joakim ;)

Cheers,
Ludwig

Am 5. Juli 2016 21:11:44 MESZ, schrieb Kees Bakker :
>Thank you for the info. It seems that gebart noticed it too :-) There
>is 
>a fresh reply in #4780.
>
>
>On 04-07-16 19:49, Joakim Nohlgård wrote:
>> Hi Kees,
>> Like Ludwig wrote, there are some places which haven't followed the 
>> coding conventions because the CC weren't as clear before as they are
>
>> now with regards to this. Since RIOT is relying on its community for 
>> code contributions, these kinds of clean ups may take a long time 
>> after the documentation has been updated until someone decides to fix
>
>> them.
>>
>> The SPI periph driver is going through some rework, see 
>> https://github.com/RIOT-OS/RIOT/pull/4780 and 
>> https://github.com/RIOT-OS/RIOT/issues/4758, but it has not yet been 
>> merged because of various other things getting in the way.
>>
>> Best regards,
>> Joakimr
>>
>> On Mon, Jul 4, 2016 at 7:23 AM, Ludwig Knüpfer 
>> > 
>> wrote:
>>
>> Hi Kees,
>>
>> Unless there is a good reason to deviate from this guideline all
>> violations should be corrected. This particular rule was added
>> relatively recently, so it would not surprise me if not all
>> occurrences in RIOT have been adapted yet.
>>
>> Cheers,
>> Ludwig
>>
>> Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker
>> >:
>> >Hi,
>> >
>> >The Coding Convention is clear about it.
>> >
>> >"Guidelines for pointer types (as long as it is reasonable):
>> >
>> >  * use |char *| for strings and only for strings
>> > * use |uint8_t[]| as type for arbitrary byte buffers, but use
>> |void *|
>> >to pass them around. |uint8_t[]| because we're dealing with
>bytes
>> >and not characters, |void *| to avoid unnecessary casting
>> shall the
>> >need arise to have their content to have a certain type
>> >  * use |uint8_t *| to pass "typed" byte buffers, e.g.,
>link-layer
>> >addresses, where it avoids unnecessary temporary variable
>> >  * use |void *| for generic typing"
>> >
>> >
>> >In the SPI driver however the transfer functions use char *
>> parameters,
>> >
>> >but SPI is usually dealing with binary
>> >information (bytes), not strings. This leads to unnecessary
>casts in
>> >other parts of the code. (E.g. nvram_spi).
>> >
>> >What is our policy about this? Are we going to correct this at
>some
>> >point? Is it too late already (I hope not)?
>>
>> ___
>> devel mailing list
>> devel@riot-os.org 
>> https://lists.riot-os.org/mailman/listinfo/devel
>>
>>
>>
>>
>> ___
>> devel mailing list
>> devel@riot-os.org
>> https://lists.riot-os.org/mailman/listinfo/devel

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Byte array should be uint8_t, not char

[Kees Bakker]

Hi Ludwig,

Well, it will be a challenge to smootly correct this.
There are 16 CPU's that use spi_transfer_byte(s) and 6 drivers.

I won't mind creating a PR, but of course I can only test it by building
examples for all boards that support SPI. And look at compile errors. Or are
there other procedures?

On 04-07-16 07:23, Ludwig Knüpfer wrote:

Hi Kees,

Unless there is a good reason to deviate from this guideline all violations 
should be corrected. This particular rule was added relatively recently, so it 
would not surprise me if not all occurrences in RIOT have been adapted yet.

Cheers,
Ludwig

Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker :

Hi,

The Coding Convention is clear about it.

"Guidelines for pointer types (as long as it is reasonable):

  * use |char *| for strings and only for strings
* use |uint8_t[]| as type for arbitrary byte buffers, but use |void *|
to pass them around. |uint8_t[]| because we're dealing with bytes
and not characters, |void *| to avoid unnecessary casting shall the
need arise to have their content to have a certain type
  * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
addresses, where it avoids unnecessary temporary variable
  * use |void *| for generic typing"


In the SPI driver however the transfer functions use char * parameters,

but SPI is usually dealing with binary
information (bytes), not strings. This leads to unnecessary casts in
other parts of the code. (E.g. nvram_spi).

What is our policy about this? Are we going to correct this at some
point? Is it too late already (I hope not)?

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel



--
Kees Bakker
Founder
SODAQ
M. 0031617737165
www.sodaq.com

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Byte array should be uint8_t, not char

[Joakim Nohlgård]

Hi Kees,
Like Ludwig wrote, there are some places which haven't followed the coding
conventions because the CC weren't as clear before as they are now with
regards to this. Since RIOT is relying on its community for code
contributions, these kinds of clean ups may take a long time after the
documentation has been updated until someone decides to fix them.

The SPI periph driver is going through some rework, see
https://github.com/RIOT-OS/RIOT/pull/4780 and
https://github.com/RIOT-OS/RIOT/issues/4758, but it has not yet been merged
because of various other things getting in the way.

Best regards,
Joakimr

On Mon, Jul 4, 2016 at 7:23 AM, Ludwig Knüpfer  wrote:

> Hi Kees,
>
> Unless there is a good reason to deviate from this guideline all
> violations should be corrected. This particular rule was added relatively
> recently, so it would not surprise me if not all occurrences in RIOT have
> been adapted yet.
>
> Cheers,
> Ludwig
>
> Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker :
> >Hi,
> >
> >The Coding Convention is clear about it.
> >
> >"Guidelines for pointer types (as long as it is reasonable):
> >
> >  * use |char *| for strings and only for strings
> > * use |uint8_t[]| as type for arbitrary byte buffers, but use |void *|
> >to pass them around. |uint8_t[]| because we're dealing with bytes
> >and not characters, |void *| to avoid unnecessary casting shall the
> >need arise to have their content to have a certain type
> >  * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
> >addresses, where it avoids unnecessary temporary variable
> >  * use |void *| for generic typing"
> >
> >
> >In the SPI driver however the transfer functions use char * parameters,
> >
> >but SPI is usually dealing with binary
> >information (bytes), not strings. This leads to unnecessary casts in
> >other parts of the code. (E.g. nvram_spi).
> >
> >What is our policy about this? Are we going to correct this at some
> >point? Is it too late already (I hope not)?
>
> ___
> devel mailing list
> devel@riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel
>
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

Re: [riot-devel] Byte array should be uint8_t, not char

[Ludwig Knüpfer]

Hi Kees,

Unless there is a good reason to deviate from this guideline all violations 
should be corrected. This particular rule was added relatively recently, so it 
would not surprise me if not all occurrences in RIOT have been adapted yet.

Cheers,
Ludwig

Am 3. Juli 2016 22:50:10 MESZ, schrieb Kees Bakker :
>Hi,
>
>The Coding Convention is clear about it.
>
>"Guidelines for pointer types (as long as it is reasonable):
>
>  * use |char *| for strings and only for strings
> * use |uint8_t[]| as type for arbitrary byte buffers, but use |void *|
>to pass them around. |uint8_t[]| because we're dealing with bytes
>and not characters, |void *| to avoid unnecessary casting shall the
>need arise to have their content to have a certain type
>  * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
>addresses, where it avoids unnecessary temporary variable
>  * use |void *| for generic typing"
>
>
>In the SPI driver however the transfer functions use char * parameters,
>
>but SPI is usually dealing with binary
>information (bytes), not strings. This leads to unnecessary casts in 
>other parts of the code. (E.g. nvram_spi).
>
>What is our policy about this? Are we going to correct this at some 
>point? Is it too late already (I hope not)?

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel

[riot-devel] Byte array should be uint8_t, not char

[Kees Bakker]

Hi,

The Coding Convention is clear about it.

"Guidelines for pointer types (as long as it is reasonable):

 * use |char *| for strings and only for strings
 * use |uint8_t[]| as type for arbitrary byte buffers, but use |void *|
   to pass them around. |uint8_t[]| because we're dealing with bytes
   and not characters, |void *| to avoid unnecessary casting shall the
   need arise to have their content to have a certain type
 * use |uint8_t *| to pass "typed" byte buffers, e.g., link-layer
   addresses, where it avoids unnecessary temporary variable
 * use |void *| for generic typing"


In the SPI driver however the transfer functions use char * parameters, 
but SPI is usually dealing with binary
information (bytes), not strings. This leads to unnecessary casts in 
other parts of the code. (E.g. nvram_spi).


What is our policy about this? Are we going to correct this at some 
point? Is it too late already (I hope not)?


--
Kees Bakker
Founder
SODAQ
M. 0031617737165
www.sodaq.com

___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel