Skip to content

sys/can: model in kconfig#19675

Closed
aabadie wants to merge 9 commits intoRIOT-OS:masterfrom
aabadie:pr/sys/can_kconfig
Closed

sys/can: model in kconfig#19675
aabadie wants to merge 9 commits intoRIOT-OS:masterfrom
aabadie:pr/sys/can_kconfig

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented May 28, 2023

Contribution description

To get the same modules loaded with and without TEST_KCONFIG, I had to fix a few dependencies with the gnrc_pktbuf shell command and STM32 periph_can.

Since conn_can has a dependency to gnrc_pkt, some modules are defined in the can Kconfig file. They should be removed once GNRC is fully migrated to Kconfig (some files already exists but seem to use an absolete strategy).

Testing procedure

  • Green CI

Issues/PRs references

Tick one item in #16875

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 28, 2023
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels May 28, 2023
@aabadie aabadie mentioned this pull request May 28, 2023
Closed
@aabadie aabadie added the CI: no fast fail don't abort PR build after first error label May 28, 2023
@riot-ci
Copy link
Copy Markdown

riot-ci commented May 28, 2023 •
edited
Loading

Murdock results

✔️ PASSED

048cc4e sys/can: remove unused module can_raw

Success Failures Total Runtime
6933 0 6933 16m:54s

Artifacts

@github-actions github-actions bot added Area: examples Area: Example Applications Area: pkg Area: External package ports Platform: native Platform: This PR/issue effects the native platform labels May 28, 2023
@aabadie aabadie force-pushed the pr/sys/can_kconfig branch from be36322 to 694bc9e Compare May 28, 2023 15:29
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented May 30, 2023

Since conn_can has a dependency to gnrc_pkt,

Is that actually what we want? It would seem this is only due to

int conn_can_isotp_recv(conn_can_isotp_t *conn, void *buf, size_t size, uint32_t timeout)
{
    assert(conn != NULL);
    assert(buf != NULL);

    int ret = 0;
    gnrc_pktsnip_t *snip;

    if (!conn->bound) {
        return -ENOTCONN;
    }

gnrc_pktsnip_t, maybe we can better isolate it? I feel like these low level things should be as isolated as possible and will help with the overall dependency modeling.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented May 30, 2023

unless this gnrc_pkt is a completely self contained utility...

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented May 30, 2023

Is that actually what we want? It would seem this is only due to

Not only, there's also

RIOT/sys/include/can/isotp.h

Lines 92 to 99 in e690ef4

struct tpcon {
unsigned idx; /**< current index in @p buf */
uint8_t state; /**< the protocol state */
uint8_t bs; /**< block size */
uint8_t sn; /**< current sequence number */
int tx_handle; /**< handle of the last sent frame */
gnrc_pktsnip_t *snip; /**< allocated snip containing data buffer */
};

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented May 30, 2023

unless this gnrc_pkt is a completely self contained utility

It is used in many places. In the CAN code, the static implementation of gnrc_pktbuf is used and in other places the malloc implementation of gnrc_pktbuf is used. This PR only covers the static implementation and the dependency resolution is done in the CAN Kconfig file (with a TODO comment to tell to move it to gnrc once it's adapted to Kconfig)

@aabadie aabadie force-pushed the pr/sys/can_kconfig branch from 694bc9e to e250d27 Compare May 30, 2023 10:12
@github-actions github-actions bot added the Area: build system Area: Build system label May 30, 2023
@aabadie aabadie force-pushed the pr/sys/can_kconfig branch 2 times, most recently from f3ec641 to b313b2f Compare May 30, 2023 10:31
@aabadie aabadie force-pushed the pr/sys/can_kconfig branch from b313b2f to 048cc4e Compare May 31, 2023 08:03
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Sep 27, 2024

no longer needed

@aabadie aabadie closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@smlng smlng Awaiting requested review from smlng

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: native Platform: This PR/issue effects the native platform

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aabadie @riot-ci @MrKevinWeiss