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

Working AT driver for MC7455 and EM7590, not tested with anything else #139

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

urielka
Copy link

@urielka urielka commented Aug 1, 2024

Seems like with sierra wireless there are 3 issues with the existing AT code:

  1. They work with logic channels that aren't integers, they use a big hex number
AT_DEBUG: AT+CCHO="A0000005591010FFFFFFFF8900000100"
AT_DEBUG: +CCHO: 00000000038aa32f

  1. The code assumes that the number returned from CCHO is the logic channel number for the eUICC and should be added to the CLA base (0x80) which results in cases sending 0x8F which seems to work with some eUICC.
    see: AT command APDU driver violates 3GPP TS 27.007 and will not work on e.g. MC7455 #138 for explanation
  2. The code checks for AT+CGLA=? running OK as a way to detect if AT+CGLA= is supported but it doesn't mean that. in EM7590 AT+CGLA= is supported and AT+CGLA=? isn't

The following code was only tested with MC7455 and EM7590. tested with esim.me sim and sysmocom (https://shop.sysmocom.de/sysmoEUICC1-eUICC-for-consumer-eSIM-RSP/sysmoEUICC1-C2G)

@urielka
Copy link
Author

urielka commented Aug 2, 2024

Reverted long usage in logic_channel_open interface as we always should return 0.

Hope now it builds on all platforms

driver/apdu/at.c Outdated Show resolved Hide resolved
Comment on lines +78 to +86
/* AT+CGLA=? might not be available but AT+CGLA= is
Sierra wireless MC7455 supports both but EM7590 only the latter
fprintf(fuart, "AT+CGLA=?\r\n");
if (at_expect(NULL, NULL))
{
fprintf(stderr, "Device missing AT+CGLA support\n");
return -1;
}

*/
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove the check on AT+CGLA=? because it doesn't work, you should add a check on AT+CGLA= or do some error handling when using AT+CGLA=.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it is that important to be honest, it will just fail after the first command, checking for AT+CGLA= requires to actually do AT+CCHO and then AT+CGLA so what are we gaining here.

Maybe change it to a warning? and again the issue is the lack of AT+CGLA= and not AT+CGLA=? where in AT commands those are separated commands so just like in the case of sierra wireless it might not be implemented

@@ -106,7 +109,7 @@ static int apdu_interface_transmit(struct euicc_ctx *ctx, uint8_t **rx, uint32_t
return -1;
}

fprintf(fuart, "AT+CGLA=%d,%u,\"", logic_channel, tx_len * 2);
fprintf(fuart, "AT+CGLA=%lx,%u,\"", logic_channel, tx_len * 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fprintf(fuart, "AT+CGLA=%lx,%u,\"", logic_channel, tx_len * 2);
fprintf(fuart, "AT+CGLA=%" PRIu ",%u,\"", logic_channel, tx_len * 2);

If you choose to use fixed-width integer type.


return logic_channel;
logic_channel = strtol(response, NULL, 16);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logic_channel = strtol(response, NULL, 16);
logic_channel = strtoull(response, NULL, 16);

Same as above.

@@ -198,7 +207,7 @@ static void apdu_interface_logic_channel_close(struct euicc_ctx *ctx, uint8_t ch
{
return;
}
fprintf(fuart, "AT+CCHC=%d\r\n", logic_channel);
fprintf(fuart, "AT+CCHC=%lx\r\n", logic_channel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fprintf(fuart, "AT+CCHC=%lx\r\n", logic_channel);
fprintf(fuart, "AT+CCHC=%" PRIu "\r\n", logic_channel);

Same as above.

Co-authored-by: Coelacanthus <CoelacanthusHex@gmail.com>
Copy link
Contributor

@septs septs left a comment

Choose a reason for hiding this comment

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

We cannot assume that all baseband modules are designed based on the MC7455 / EM7590 AT command set.

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